Fix paper-slider pin style #1022
Conversation
|
I am impressive with how you elegantly hacked this together. However, I also think that this guaranteed will break in the future. I know they are in feature freeze now, but won't be forever. I think that we should just copy paste the component into our repo and make the changes. |
|
That way it will also be a bit harder to get bug fixes. |
|
Let's fork. |
src/components/ha-paper-slider.html
Outdated
| </template> | ||
|
|
||
| <script> | ||
| customElements.whenDefined('paper-slider').then(() => { |
There was a problem hiding this comment.
Why do you need "whenDefined"? There is no async here, as you import paper-slider.
There was a problem hiding this comment.
See this comment. Polymer/polymer#4556 (comment)
But maybe I misunderstood that.
But we don’t need this anyway because we’ll clone the whole element instead of extending it (see balloobs comment above.)
There was a problem hiding this comment.
Just for the record, according to Polymer/polymer#4556 (comment) whenDefined wasn't needed. (Unless used from the index doc, which AFAIU is not our case)
src/components/ha-paper-slider.html
Outdated
| static get template() { | ||
| if (!myTemplate) { | ||
| // Retrieve this element's dom-module template | ||
| myTemplate = Polymer.DomModule.import(this.is, 'template'); |
There was a problem hiding this comment.
I think HaPaperSlider.is is more clear than this.is in a static context.
There was a problem hiding this comment.
In this case the target is actually to get the parents class (PaperSlider) template.
So I think then it it would be more clear to use PaperSlider.is
P.S. I like little discussions like this, I learn a lot from them 👍
There was a problem hiding this comment.
You are right. I responded without taking a good look at the code. Fixed.
|
@andrey-git what do you think we should do, fork or compose a new component off the old one on the fly? |
|
I think generally extending is better. If we see that an update breaks us - we can work then. |
|
Alright, let's not fork and keep the current approach. |
|
@armills I could use a little help with the JSdoc directives ;) |
|
I think the problem is that polymer lint does not know the type when you fetch the class from the registry. |
|
And so it broke… |
|
|
|
Issue #1190 |
This was supposed to be a simple fix for two simple shortcomings:
However, paper-elements is currently in feature-ice-age (see: PolymerElements/paper-slider#212)
So I extended paper-slider via the following methods:
now we have an almost exact copy of paper-slider, with its exact DOM template but a modified <style> element in it.
Fixes: #390