[ignore] list-item: colour indicator #3909
Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
|
There was a problem hiding this comment.
@dbatiste this covers the most basic implementation, but just wanted to see if you had any thoughts at this point before I get into the more complicated cases.
On the demo site the changes can be seen on the list demo and the list-expand-collapse demo
| @@ -197,6 +197,7 @@ class ListDemoNested extends LitElement { | |||
| return html` | |||
| <d2l-list-item | |||
| action-href="${this.includeActionHref ? 'http://www.d2l.com' : ''}" | |||
| color="#ffaa47" | |||
There was a problem hiding this comment.
Thoughts on if the # should be included by the consumer or if we should be adding that?
There was a problem hiding this comment.
My first thought is that they should be required to provide the # and we shouldn't assume anything. Although we might decide to limit this to hex colors, technically we could allow rgb, rgba, named colors, CSS properties, etc. and we probably shouldn't assume they'll never be accepted.
| @@ -415,6 +440,9 @@ export const ListItemMixin = superclass => class extends composeMixins( | |||
| if (changedProperties.has('breakpoints')) { | |||
| this.resizedCallback(this.offsetWidth); | |||
| } | |||
| if (changedProperties.has('color')) { | |||
| this.shadowRoot.querySelector('.d2l-list-item-color-inner').style.backgroundColor = this.color; | |||
There was a problem hiding this comment.
Should we do any verification that this is a hex code or rely on the consumer to handle that?
There was a problem hiding this comment.
That's a good question. The browser will ignore anything that is completely invalid. Having said that, if an invalid color is being set here, then it would be good to know so that we can correct the source. If we throw an error then I think it will get logged.
Aside from that, I do tend to lean towards limiting this to hex values to start, since I think the source of the colors will always produce hex colors. I would also be interested in Jeff's input too though.
There was a problem hiding this comment.
I'd suggest doing the same thing we do in the color input and validate in the setter -- we could even reuse the regex.
| @@ -67,6 +67,11 @@ export const ListItemMixin = superclass => class extends composeMixins( | |||
| * @type {array} | |||
| */ | |||
| breakpoints: { type: Array }, | |||
| /** | |||
| * A color indicator to appear at the beginning of a list item. Expected value is a valid color hex code (e.g., '#006fbf'). | |||
There was a problem hiding this comment.
Hex values could also include the alpha channel. ex. #RRGGBBAA for semi-transparent colors. I'm not sure if they will try to use semi-transparent colors, or whether we want to actively stop them from doing so. That's probably a follow-up for the designers, and depending on what they say we might want to update this doc comment.
Related: Although it is more work for the list/list-items to do, I think we need the list components to automatically adjust spacing based on whether other items in the same list have color indicators rendered. Allowing consumers to have any influence over this multiplies the nuttiness of all the spacing permutations that Jeff is trying to sort out. So, assuming that, the consumer shouldn't ever have a need for a fully transparent color (transparent, rgba(R,G,B,0), #RRGGBB00).
|
Thanks for the feedback! Closing this PR, and will be opening new PRs with the different pieces of this functionality. |
No description provided.