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(postcss-colormin): Strict color parsing #1122

Merged
merged 4 commits into from May 21, 2021

Conversation

omgovich
Copy link
Contributor

@omgovich omgovich commented May 21, 2021

Fixes #1120
Fixes #1115
Fixes #1113

Colord v2 conforms to CSS Color Level Specs and ignores invalid <color> values, so upgrading will fix all of the issues we got last few days.

See omgovich/colord#53 for more details.

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.

Let's add tests to ensure we don't break it again 👍

@omgovich
Copy link
Contributor Author

How do you work with the integration tests BTW? They are HUGE and it seems impossible to check them.
They are falling (probably because they were updated during the migration to color v1.7) but I can't even see where is the error.

@alexander-akait
Copy link
Member

How do you work with the integration tests BTW? They are HUGE and it seems impossible to check them.

Agree, we need improve them, right now you can use smart diff locally and found which characters was changed (and maybe found regression)

@omgovich
Copy link
Contributor Author

omgovich commented May 21, 2021

Ooof. Your integrations tests are killing me 🙃

  1. What is smart diff? Can't google the thing)
  2. How to update integration snapshots properly?

Help would be really appreciated since I'm a rookie in the repo.

@ludofischer
Copy link
Collaborator

ludofischer commented May 21, 2021 via email

@omgovich
Copy link
Contributor Author

omgovich commented May 21, 2021

Snapshots are fixed (tons of z-index: #999 were fixed there).
New tests were also added.

So we should be ready to go!

@codecov-commenter
Copy link

codecov-commenter commented May 21, 2021

Codecov Report

Merging #1122 (378a687) into master (8a31ca3) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1122      +/-   ##
==========================================
- Coverage   96.37%   96.36%   -0.01%     
==========================================
  Files         115      115              
  Lines        3582     3575       -7     
  Branches     1054     1051       -3     
==========================================
- Hits         3452     3445       -7     
  Misses        121      121              
  Partials        9        9              
Impacted Files Coverage Δ
packages/postcss-colormin/src/colours.js 100.00% <100.00%> (ø)

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 8a31ca3...378a687. Read the comment docs.

@alexander-akait
Copy link
Member

/cc @ludofischer can you look too? I think we need do release after merge

Copy link
Collaborator

@ludofischer ludofischer left a comment

Choose a reason for hiding this comment

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

Seems straightforward enough to me. Let's try and release this!

@ludofischer ludofischer merged commit 32771da into cssnano:master May 21, 2021
@ludofischer
Copy link
Collaborator

Thanks a lot @omgovich for your hard work!

@michaelfaith
Copy link

Thanks for the quick work on this guys.

@omgovich omgovich deleted the fix/strict-color-parsing branch May 21, 2021 17:27
@omgovich
Copy link
Contributor Author

Always glad to help. Let me know if you need something else :)

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