search

eBay / skin

Pure CSS framework designed & developed by eBay for a branded, e-commerce marketplace.
https://ebay.github.io/skin/
MIT License
178 stars 67 forks source link

Icon: css cascade order issue for foreground icons when using `@ebay/skin/icon` #540

Closed ianmcburnie closed 5 years ago

ianmcburnie commented 5 years ago

Here is the current browser.json for @ebay/skin/icon:

{
    "dependencies": [
        "@ebay/skin/icon/background",
        "@ebay/skin/icon/foreground",
        {
            "if-not-flag": "skin-ds6",
            "dependencies": [
                "@ebay/skin/iconfont"
            ]
        }
    ]
}

NOTE: In 7.0.0 we are removing iconfont.

Here is @ebay/skin/icon/background:

{
    "dependencies": [
        {
            "path": "../dist/icon/background/ds4/background.css",
            "if-not-flag": "skin-ds6"
        },
        {
            "path": "../dist/icon/background/ds6/background.css",
            "if-flag": "skin-ds6"
        },
        "@ebay/skin/icon/common"
    ]
}

Here is @ebay/skin/icon/foreground:

{
    "dependencies": [
        {
            "path": "../dist/icon/foreground/ds4/foreground.css",
            "if-not-flag": "skin-ds6"
        },
        {
            "path": "../dist/icon/foreground/ds6/foreground.css",
            "if-flag": "skin-ds6"
        },
        "@ebay/skin/icon/common"
    ]
}

The lasso resolution will go:

  1. background.css
  2. common.css
  3. foreground.css
  4. common.css (but lasso will dedupe this duplicate include)

Users of background icons will be fine, but specific foreground icons will have their width and height trumped due to base foreground.css rules coming after common.css.

This is therefore a regression for users of foreground icons.

ianmcburnie commented 5 years ago

Going to work on a 6.3.5 patch for this issue. I may have to remove the concept of "common" and just keep background and foreground completely separate.

seangates commented 5 years ago

You could still keep the common stuff, but have it be part of the LESS import, and not part of the Lasso bundling.

ianmcburnie commented 5 years ago

Right, but in the dist css, there would be no concept of shared/common css for the icon widths and heights. Both background.css and foreground.css would include all of that (which is what I was trying to originally avoid). I don't think it's a big deal though, and solves this problem for now in a backwards compatible way.

ianmcburnie commented 5 years ago

Shame Lasso doesn't have a "don't dedupe this module" ability...or does it? That would be a quick and dirty fix for this, until I could refactor properly in 7.0.0. @DylanPiercey ?

Assuming not, I'm going to do this as a two part fix:

  1. "just get it working fix", backwards compatible with minimum refactoring and moving of files. Will definitely not produce optimal css.
  2. "proper fix", breaking change with refactoring and moving some files around. will output more optimal css.