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

chore: update css-declaration-sorter to v6.0.3 #1071

Merged
merged 6 commits into from May 17, 2021
Merged

chore: update css-declaration-sorter to v6.0.3 #1071

merged 6 commits into from May 17, 2021

Conversation

Siilwyn
Copy link
Contributor

@Siilwyn Siilwyn commented May 11, 2021

Update so Node.js 10 is supported in combination with PostCSS 8.
#1065 (comment)

@Siilwyn
Copy link
Contributor Author

Siilwyn commented May 11, 2021

Seems like some prefixed properties like -webkit-text-size-adjust and other properties like text-rendering shifted around. 🤔

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2021

Codecov Report

Merging #1071 (c83de2a) into master (02e3aab) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1071   +/-   ##
=======================================
  Coverage   96.37%   96.37%           
=======================================
  Files         115      115           
  Lines        3589     3589           
  Branches     1059     1059           
=======================================
  Hits         3459     3459           
  Misses        121      121           
  Partials        9        9           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02e3aab...c83de2a. Read the comment docs.

@ludofischer
Copy link
Collaborator

Seems like some prefixed properties like -webkit-text-size-adjust and other properties like text-rendering shifted around. thinking

I think it would be a good idea to add some tests to https://github.com/cssnano/cssnano/blob/master/packages/cssnano-preset-default/src/__tests__/css-declaration-sorter.js, for example to check that integrating the new css-declaration-sorter release fixes #1054

@alexander-akait
Copy link
Member

Agree 👍

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add test for all?

@Siilwyn
Copy link
Contributor Author

Siilwyn commented May 13, 2021

Rebased and added four tests, one test has weird output not sure what to do with it, maybe something to put into knownIssues.js?

    Expected: "a{border-top:1px solid;border-color:purple}"
    Received: "a{border-color:currentcolor purple purple;border-top:1px solid purple}"

@alexander-akait
Copy link
Member

Yep, weird output, maybe problem due order of plugin?

@Siilwyn
Copy link
Contributor Author

Siilwyn commented May 13, 2021

@alexander-akait I did try removing css-declaration-sorter but the test fails with the same output, maybe it's a bug in the merge-longhand logic?

Also the framework integration tests keep failing now, even though I did run yarn build:integration.

@alexander-akait
Copy link
Member

@alexander-akait I did try removing css-declaration-sorter but the test fails with the same output, maybe it's a bug in the merge-longhand logic?

let's skip it and open an issue for this 👍

@Siilwyn
Copy link
Contributor Author

Siilwyn commented May 13, 2021

@alexander-akait alright, it's odd though if I run the same input in the cssnano playground it doesn't have this behaviour.

@alexander-akait
Copy link
Member

Need fix CI

@ludofischer
Copy link
Collaborator

Need fix CI

Probably need to regenerate the integration tests

yarn build:integration

@Siilwyn
Copy link
Contributor Author

Siilwyn commented May 15, 2021

Need fix CI

Probably need to regenerate the integration tests

yarn build:integration

Yes as I said before:

Also the framework integration tests keep failing now, even though I did run yarn build:integration.

So even after running this command there are no changes, any help appreciated.

@alexander-akait
Copy link
Member

/cc @ludofischer Can you look? Maybe we have problems with tests...

@Siilwyn
Copy link
Contributor Author

Siilwyn commented May 15, 2021

I rebased, and ran yarn build:integration just to be sure.

@ludofischer
Copy link
Collaborator

I rebased, and ran yarn build:integration just to be sure.

But did you commit the modified CSS fixtures? They're in packages/cssnano-preset-default/src/__tests__/integrations/ and packages/cssnano-preset-advanced/src/__tests__/integrations/. I still see the integration tests failing on CI.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented May 15, 2021

I rebased, and ran yarn build:integration just to be sure.

But did you commit the modified CSS fixtures? They're in packages/cssnano-preset-default/src/__tests__/integrations/ and packages/cssnano-preset-advanced/src/__tests__/integrations/. I still see the integration tests failing on CI.

The CSS fixtures stay the same, so nothing to commit. Just to be 200% sure I removed all files in the directories you mention manually before running the build:integration script.

@ludofischer
Copy link
Collaborator

The CSS fixtures stay the same, so nothing to commit. Just to be 200% sure I removed all files in the directories you mention manually before running the build:integration script.

Do the tests pass locally? Because in the test logs the result does not match the fixture.

@ludofischer
Copy link
Collaborator

I've opened #1071 to add a commit with the fixtures fixed. What I think happened is that the tests testthe code in package/src but the fixtures are generated with the code in dist so you need to run yarn to rebuild all packages before running build:integration.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented May 15, 2021

Alright let me know where to 'continue' this PR, I've cherry picked 4ec0f00. Can also close this one. Thanks for the help!

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

Successfully merging this pull request may close these issues.

None yet

4 participants