Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add markdown to persistent notifications #128

Merged
merged 17 commits into from
Oct 18, 2016
Merged

Add markdown to persistent notifications #128

merged 17 commits into from
Oct 18, 2016

Conversation

justweb1
Copy link
Contributor

@justweb1 justweb1 commented Oct 14, 2016

This is still a WIP and I have not tested just yet. I wanted to leave it here for feedback from @balloob before I go much further. @kellerza I could use your help modifying the backend to include markdown on the demo for testing.

Related Issue
#123

Related Pull Request
home-assistant/core#3908
home-assistant/core#3919

@mention-bot
Copy link

@justweb1, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob to be a potential reviewer.

@balloob
Copy link
Member

balloob commented Oct 14, 2016

This will not work. Scripts need to be included via bower and importHref need a url that works, this won't work in prod.

I don't have too much time lately so I won't have time to hand hold you through this PR.

@justweb1
Copy link
Contributor Author

No problem I will add through bower and get it working I ran out of time to
work on it this morning and need to get the demo updated with markdown to
test it.

On Fri, Oct 14, 2016, 10:35 AM Paulus Schoutsen [email protected]
wrote:

This will not work. Scripts need to be included via bower and importHref
need a url that works, this won't work in prod.

I don't have too much time lately so I won't have time to hand hold you
through this PR.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#128 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AKviteDqQsERW8H9rLjceFbrQon68SRcks5qz6FQgaJpZM4KXAl6
.

@justweb1
Copy link
Contributor Author

I think I have the frontend ready. Just need some help from @kellerza on the backend so we can do some testing.

@justweb1
Copy link
Contributor Author

Closing pull request for now, will resubmit once its ready.

@justweb1 justweb1 closed this Oct 16, 2016
@justweb1 justweb1 reopened this Oct 16, 2016
@justweb1
Copy link
Contributor Author

@balloob This is working now. There are a few items to note.

  1. Some of the CSS doesn't seem to work properly. I'm not really sure how to address that.
  2. This does leave an security vulnerability if persistent notification gets created from an untrusted source like hass deployments with remote access but no ssh.
  3. Travis is throwing an error for micromarkdown not being defined but it is defined in an external script so I don't think that will clear.

@justweb1 justweb1 changed the title [WIP] Add markdown to persistent notifications Add markdown to persistent notifications Oct 16, 2016
@@ -0,0 +1,6 @@
<script
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this file and it's dependencies and put it in the static folder inside Home Assistant. The polymer dir and bower_components dir are not included in production.

value: false,
},

loaded: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have this parameter, this is always exactly the same as scriptLoaded

var el = '';
var message = '';
if (loaded) {
el = document.getElementById('pnContent');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Polymer will make all DOM elements with an id available at this.$, so use this.$.pnContent. It's better that way because otherwise you would only be ever able to have 1 card with markdown syntax

@@ -18,6 +18,7 @@
"paper-scroll-header-panel": "~1.0.16",
"app-layout": "~0.10.2",
"fecha": "~2.2.0",
"paper-range-slider": "IftachSadeh/paper-range-slider#~0.1.2"
"paper-range-slider": "IftachSadeh/paper-range-slider#~0.1.2",
"micromarkdown.js": "SimonWaldherr/micromarkdown.js#~0.3.0"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to add this to bower.json since we don't import it. However, please include the version number and license inside the new micromarkdown.js folder in Home Assistant repo.

@justweb1
Copy link
Contributor Author

@balloob This should be ready to merge.

Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok to merge when comments addressed.

};

this.importHref(
'/static/micromarkdown/micromarkdown-js.html',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script is not located in a micromarkdown folder.

@justweb1 justweb1 merged commit 78c7756 into home-assistant:master Oct 18, 2016
var message = '';
if (scriptLoaded) {
el = this.$.pnContent;
message = micromarkdown.parse(stateObj.state);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@justweb1 Can we try to pass a second parameter of 'True' here? It might fix relative links SimonWaldherr/micromarkdown.js#29

tkdrob pushed a commit to tkdrob/frontend that referenced this pull request Apr 20, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jul 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants