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

Add color-adjust declaration support #1009

Merged
merged 8 commits into from Mar 21, 2018
Merged

Add color-adjust declaration support #1009

merged 8 commits into from Mar 21, 2018

Conversation

@soul-wish
Copy link
Contributor

@soul-wish soul-wish commented Mar 20, 2018

Here we go in fixing #1001 issue. @ai, can you check this PR, please? Because I'm not sure about some modifications I've made. Haven't found documentation or contribution guide for Autoprefixer, unfortunately. What do you think about adding such type of documentation/guide in the future? Even simple CONTRIBUTING.md file would be helpful for newcomers.
I'll try to leave some comments in my commit below ;)

// CSS color-adjust property
f(require('caniuse-lite/data/features/css-color-adjust.json'), browsers =>
prefix(['color-adjust'], {
mistakes: ['-khtml-', '-ms-', '-moz-', '-o-'],

This comment has been minimized.

@soul-wish

soul-wish Mar 20, 2018
Author Contributor

If I got it in the correct way, this array prevents Autoprefixer from adding unnecessary prefixes. In our case there is no need to add any prefixes except -webkit-.

This comment has been minimized.

@ai

ai Mar 20, 2018
Member

Not, it is not correct. Check other prefixes, they don’t have mistakes. mistakes is only for popular mistakes. I think nobody wrote -khtml-color-adjust in the real world.

@@ -144,7 +144,7 @@ const COMMONS = [
'advanced-filter', 'element', 'image-set', 'image-rendering',
'mask-border', 'writing-mode', 'cross-fade', 'gradient-fix',
'text-emphasis-position', 'grid', 'grid-area', 'grid-template',
'grid-template-areas'
'grid-template-areas', 'color-adjust'

This comment has been minimized.

@soul-wish

soul-wish Mar 20, 2018
Author Contributor

Should I add 'color-adjust' declaration to the array of COMMONS? Not sure about this change. It was a little hard to get the whole structure and logic inside this test file.

This comment has been minimized.

@ai

ai Mar 20, 2018
Member

Yes, you should add it to COMMONS to run color-adjust with not-duplication and prefixes removing tests

@@ -422,6 +422,7 @@ describe('hacks', () => {
it('supports text-decoration', () => test('text-decoration'));
it('ignores modern direction', () => test('animation'));
it('supports overscroll-behavior', () => test('overscroll-behavior'));
it('supports webkit prefixes', () => test('color-adjust'));

This comment has been minimized.

@soul-wish

soul-wish Mar 20, 2018
Author Contributor

Is the name of test is ok? I tried to get the correct naming logic from the test cases above.

This comment has been minimized.

@ai

ai Mar 20, 2018
Member

Nope, the test name is incorrect. Check others, they contain hack name, not prefix.

@soul-wish
Copy link
Contributor Author

@soul-wish soul-wish commented Mar 20, 2018

@ai thanks for all your help! I've updated PR according to your instructions. Let me know if I've missed something.

Yuriy Alekseyev and others added 2 commits Mar 20, 2018
… to names array in color-adjust.js, change html elements to class names and fix indentation in color-adjust.out.css
Necessary additions
@soul-wish
Copy link
Contributor Author

@soul-wish soul-wish commented Mar 21, 2018

@ai, @yuriyalekseyev found some issues in original PR, so here is updated version. 😉

@yuriyalekseyev
Copy link
Contributor

@yuriyalekseyev yuriyalekseyev commented Mar 21, 2018

@soul-wish @ai it seems that tests are green with a new version of caniuse 👍

yuriyalekseyev and others added 3 commits Mar 21, 2018
Upgrade caniuse-lite version
@soul-wish
Copy link
Contributor Author

@soul-wish soul-wish commented Mar 21, 2018

Now we have issue only with size-limit package and increased package size. I hope @ai will tell us what do do with this issue (not sure we need to modify size-limit configuration inside our PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants