Icon: missing disabled chevron icons (DS4)

[swathip88]

icons for icon-chevron-light-left-disabled, icon-chevron-light-right-disabled, icon-chevron-left-disabled, icon-chevron-right-disabled

[ianmcburnie]

As these were not part of DS4 and are not part of DS6 or "one design", it is unlikely we will be adding these now ourselves just for DS4. PRs are accepted though.

[seangates]

@ianmcburnie I actually think she means chevron-left-disabled.

[swathip88]

@ianmcburnie Sorry edited the description

[ianmcburnie]

No worries. It's probably the same answer though, right? Unless these are going to be added to DS6 icon library, then I don't see us adding them just for DS4.

[seangates]

Looking at the pagination component (Which is where this question is rooted), it’s using the background icons right now. No bueno.

https://github.com/eBay/ebayui-core/blob/master/src/components/ebay-pagination/template.marko#L24

[ianmcburnie]

Sorry @seangates but that comment is only adding to my confusion. Let's please be clear about what is required in Skin and why. The original description is not very clear, so if you have any additional context to clear this up, please share.

Digging deeper myself I see that these four are present in DS4 icon-variables.less, but NOT in DS4 icons.svg or base64-variables.less:

• icon-chevron-light-left-disabled
• icon-chevron-light-right-disabled
• icon-chevron-left-disabled
• icon-chevron-right-disabled

I don't know why we now have base64 declared in two places, base64-variables and icon-variables but they seem at odds with each other. So I would say the task is to add these four icons to DS4 icons.svg and have a single source of truth for the base64 in DS4 and DS6.

@seangates is that correct?

@agliga If so, can you take a look please?

[seangates]

@ianmcburnie My shallow look at DS4 showed there was no Base64 background icons, which is what is needed. I didn't dive deep, but asked @swathip88 to create a ticket.

Also, my comment about the carousel component using background SVG was simply as part of looking at the Core/Skin code briefly.

Apologies on any confusion.

[ianmcburnie]

Okay, it really wasn't clear if this was background icons or foreground icons.

@agliga Can take a look and see if my assumptions (https://github.com/eBay/skin/issues/861#issuecomment-535584609) are correct. I'm especially curious about the duplication of base64 across those two files...

[agliga]

@ianmcburnie So the generated ds4 file should not be used yet. I created it, but I did not go through it thoroughly and verify that it works. I left it in for now, but I need to go through and make sure all the icons work and that there is no regression. I left it out for now since I didn't want to regress ds4 On the other hand, I went through the ds6 file and did remove a lot of icons. Eventually we shouldn't have any base64 in the icon's file.

[agliga]

@seangates @ianmcburnie So it looks like there are base64 disabled icons which should be used properly. Again, the base64-icons.less ds4 file is not being imported so that's not even being used. There aren't any disabled svg icons, so that might be the problem. However, shouldn't we just use CSS classes to disable the icons since they are foreground icons?

[ianmcburnie]

@agliga @seangates Shouldn’t we just be applying a fill where the "regular" symbol is used? Rather than duplicating the symbol for the disabled state?

I see we have a disabled symbol in the case below, but note the path definition is identical. It seems wasteful to do this everytime we need a "disabled" version. Or maybe the developer convenience is worth it? Thoughts?

<symbol id="icon-arrow-down" viewBox="0 0 24 13">
  <path d="M11.604 12.921L0 0h23.215l-5.807 6.462z"/>
</symbol>

<symbol id="icon-arrow-down-disabled" viewBox="0 0 24 13">
  <path fill="#ccc" d="M11.604 12.921L0 0h23.215l-5.807 6.462z"/>
</symbol>