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

fix(cssnano-preset-default): update css-declaration-sorter #1521

Closed
wants to merge 1 commit into from

Conversation

Siilwyn
Copy link
Contributor

@Siilwyn Siilwyn commented Sep 23, 2023

7.1.0 includes a fix to not change the order of unknown properties to
avoid mixing unknown shorthand and longhand properties.

Closes #1514

Because the snapshots contain some experimental properties which are not included in the sorter I've updated them.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Sep 23, 2023

Will fix the TS error soonish, I'll have to publish another version bump.

@codecov
Copy link

codecov bot commented Oct 1, 2023

Codecov Report

All modified lines are covered by tests ✅

📢 Thoughts on this report? Let us know!.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Oct 1, 2023

Fixed the regression with commonjs types resolution, ready for review!

7.1.0 includes a fix to not change the order of unknown properties to
avoid mixing unknown shorthand and longhand properties.

Closes cssnano#1514
@ludofischer
Copy link
Collaborator

Fixed the regression with commonjs types resolution, ready for review!

Did you look at the changes in the CSS files to make sure they look as expected? I know it can be quite annoying to inspect those diffs.

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Oct 1, 2023

@ludofischer yes I've looked through them, to be honest there is one unexpected change in blueprint css where max-width and flex-basis is swapped, I can not reproduce this in a test though.

E.g.

test(
  'This works',
  passthroughCSS('a{flex-basis:8.33333%;max-width:8.33333%}')
)

@ludofischer
Copy link
Collaborator

@ludofischer yes I've looked through them, to be honest there is one unexpected change in blueprint css where max-width and flex-basis is swapped, I can not reproduce this in a test though.

There are other changes I do not understand. For example, why does font-family now move before -ms-size-adjust (in base-v0.1.0)?

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Nov 2, 2023

@ludofischer sorry for the late reply, I'm back from vacation and took some time to get used to life & work rhythm.

It's an odd change, could it be the combination of other modules causing this?

If I add a test it passes:

test(
  'keep order',
  passthroughCSS('html{-ms-text-size-adjust:100%;-webkit-text-size-adjust:100%;font-family:sans-serif}')
);

@ludofischer
Copy link
Collaborator

It's odd indeed. Do any unwanted consequences come to mind? How could it be other modules if nothing changed in this PR except css-declaration-sorter?

@ludofischer
Copy link
Collaborator

Hi, I've reapplied the changes myself in a different branch because a SVGO upgrade in-between had caused too many conflicts in the CSS files

@Siilwyn
Copy link
Contributor Author

Siilwyn commented Dec 18, 2023

Ah I see thank you @ludofischer, sorry for not replying been busy again.
❤️

@Siilwyn Siilwyn deleted the update-css-sorter branch December 18, 2023 16:37
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.

[Bug]: Sorts animation-longhands before animation-shorthand
2 participants