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

`_conflict` should get handled differently #110

Closed
JPeer264 opened this issue Mar 5, 2020 · 4 comments
Closed

`_conflict` should get handled differently #110

JPeer264 opened this issue Mar 5, 2020 · 4 comments
Labels
bug

Comments

@JPeer264
Copy link
Owner

@JPeer264 JPeer264 commented Mar 5, 2020

Sometimes in JS files things get concatenated like:

const myAwesomeNumber = 200;
const newDistanceBetween = myAwesomeNumber + 'px';

So when there was already a selector which got renamed to px the whole code will get transformed to

const myAwesomeNumber = 200;
const newDistanceBetween = myAwesomeNumber + 'px_conflict';

Which is not really correct and would end up breaking the code. I have three suggestions:

  1. Removing _conflict and just add a warning of all "conflicting" selectors (also add them into the stats
  2. Removing _conflict but add a parameter to allow to add this markConflictSelectors (or similar)
  3. Leaving _conflict but suggest in the readme to ignore those selectors

@X-Ryl669 do you have any suggestions on this?

@JPeer264 JPeer264 added the bug label Mar 5, 2020
@X-Ryl669

This comment has been minimized.

Copy link
Contributor

@X-Ryl669 X-Ryl669 commented Mar 5, 2020

What I did in my case (almost similar, and due to, for example: var a = 'title';) is to exclude all of them.

I don't know if you remember the Caveat.md file I've pushed in rename-css-selector repository at the time.

Typically, I think it might be worth to exclude all known conflicting patterns, and they are not legion. Typically, it's HTML tag's name + CSS unit's name. We'll loose a bit of potential compression, but safety goes up and likely justify them.
I can provide my list if you need (the one I'm using in my projects).

So instead of creating an empty exclude list, we can just populate a default exclude list and let the user remove them if she wants to (most won't).

@JPeer264

This comment has been minimized.

Copy link
Owner Author

@JPeer264 JPeer264 commented Mar 5, 2020

Ya I remember! (thanks for reminding, I still got the files in the dev branch)

It would be really nice if you could provide your list and we add it to the core.

@X-Ryl669

This comment has been minimized.

Copy link
Contributor

@X-Ryl669 X-Ryl669 commented Mar 5, 2020

It's this:

{
    "exclude": [
"html", "head", "title", "base", "link", "meta", "style", "body", "article", "section", "nav", "aside", 
"h1", "h2", "h3", "h4", "h5", "h6", "hgroup", "address", "p", "hr", "pre", "blockquote", "ol", "ul", 
"menu", "li", "dl", "dt", "dd", "figure", "figcaption", "main", "div", "a", "em", "strong", "small", 
"s", "cite", "q", "dfn", "abbr", "ruby", "rt", "rp", "data", "time", "code", "var", "samp", "kbd", 
"sub", "sup", "i", "b", "u", "mark", "bdi", "bdo", "span", "br", "wbr", "ins", "del", "picture", 
"source", "img", "iframe", "embed", "object", "param", "video", "audio", "track", "map", "area", 
"table", "caption", "colgroup", "col", "tbody", "thead", "tfoot", "tr", "td", "th", "form", "label", 
"input", "button", "select", "datalist", "optgroup", "option", "textarea", "output", "progress", "meter", 
"fieldset", "legend", "details", "summary", "dialog", "script", "noscript", "template", "slot", "canvas",

"em", "ex", "px", "cm", "mm", "in", "pt", "pc", "ch", "rem", "vmax", "vmin", "vw", "vh", 
"currentcolor"
]
}

It's sad there is no possible comment in the file, but basically, the first group of words are HTML5 tagname and the second group are CSS units, the last group is a CSS specific word referring to the current color (never used that myself, but included for sake of completeness).
I've not included all CSS functions like rgb or calc since they need to be used with a parenthesis and because of this, the core will not infer it's a selector anymore and won't replace it.

Please notice that, contrary to HTML4, HTML5 allow any tag name so this list can not be exhaustive but at least, it covers ~99% of use cases

@JPeer264

This comment has been minimized.

Copy link
Owner Author

@JPeer264 JPeer264 commented Mar 5, 2020

Awesome thanks a lot 👍

@JPeer264 JPeer264 closed this in bbd93bc Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.