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
fix: fixed merging of unsupported pseudos by caniuse and browserlist #883
base: master
Are you sure you want to change the base?
fix: fixed merging of unsupported pseudos by caniuse and browserlist #883
Conversation
Codecov Report
@@ Coverage Diff @@
## master #883 +/- ##
==========================================
- Coverage 97.27% 97.07% -0.21%
==========================================
Files 118 118
Lines 3452 3454 +2
Branches 1036 1037 +1
==========================================
- Hits 3358 3353 -5
- Misses 86 93 +7
Partials 8 8
Continue to review full report at Codecov.
|
cc47223
to
8fb8163
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, what is changed in snapshots?
Integration fixtures. there where some of the places in the framework fixtures where it was merging unsupported rules. Output of |
Can you look on snapshot changes, and write them here (no need all, just couple) |
for It didnt merge the
|
Can we fix |
https://caniuse.com/#feat=mdn-css_selectors_visited, think we should try |
I will look on visited in near future |
Codecov Report
@@ Coverage Diff @@
## master #883 +/- ##
==========================================
- Coverage 97.28% 97.12% -0.16%
==========================================
Files 118 119 +1
Lines 3458 3479 +21
Branches 1040 1048 +8
==========================================
+ Hits 3364 3379 +15
- Misses 86 92 +6
Partials 8 8
Continue to review full report at Codecov.
|
'::grammar-error': 'grammar-error', | ||
|
||
'::part()': '::part', | ||
'::slotted()': '::slotted', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why some have ()
parenthesis, some doesn't have? It is bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok these should not have ()
but these are level 4 CSS Selector
except for the visited
.
And I think level 4 is not supported by caniuse-lite
, so this all will be removed
I think, |
7f349fe
to
448455a
Compare
@anikethsaha Yes 👍 |
there were level 4 CSS selectors in early commits of this PR, I had to remove them as |
@anikethsaha So we do not merge selector for 4 level? |
currently no! Cause we dont know their browser support so that may lead to wrong minification. |
We have some of them https://caniuse.com/#search=selector%204 |
I guess I couldn't locate the selectors here. (may be I missed something, let me know) |
ohh great, I thought they might have packed like level 2,3 are I will add those which are supported. |
':visited': cssSel3, | ||
':any-link': 'css-any-link', | ||
':read-only': 'css-read-only-write', | ||
':read-write': 'css-read-only-write', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can have more pseudo here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I all covered the level 4. let me check if I missed others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we create script to generate this stuff from caniuse-api
? It is allow to validate we nothing is missed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i will check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I am afraid we can missing something, so we need script to generate and validate it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what I am thinking is to simply query using the code name like for :react-write
, query using react-write
. but two issue can come
- there may be more that one result, and we dont know which is correct
- there may be no result as the query can be different
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can parse all existing pseudo from css-whatwg
?
Need to check each |
|
Also, those verdor prefixed selectors will be un-prefixed using the postcss api here. |
I think continuing this work could also fix #999 |
This change won't merge rules (having pseudo-classes) which are not either supported by caniuse in browserlist 's browsers.
The bug was even they were not supported, the compatibility was not changed which leads to false merging.