-
-
Notifications
You must be signed in to change notification settings - Fork 929
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 lightness-notation #7366
Add lightness-notation #7366
Conversation
|
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.
Thank you for the fixes @fpetrakov
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.
@fpetrakov Thanks for creating the PR. I don't believe there are major problems. 👍🏼
Can you take a look at my suggestion comments? If you have rationale, you don't need to address all.
Note: I'd like to merge this PR after the 16.0.0 release (#7374). |
@ybiquitous Thanks for reviewing the PR. All of yours suggestions are sensible, so I will fix things in the coming days |
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, LGTM. I'll revisit this after the 16.0.0 release.
Also, we need to determine a value (maybe percentage
?) added in the standard config before publishing this new rule.
I agree on |
@fpetrakov Can you resolve the conflicts raised by the 16.0.0 release? |
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.
Thank you! It's looking great.
I've requested a change to the README, otherwise LGTM.
dd0dff5
to
460984d
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.
Thanks, LGTM 👍🏼
One more request, can you add a changelog entry via npx changeset add
?
Ref https://github.com/stylelint/stylelint/blob/e28036e8c6121e2293a57986998af941ef2acc01/.changeset/early-jobs-hunt.md?plain=1
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.
minor
Maybe add a fix case using deg
.
Or change one or two existing ones to use deg
.
i.e. to detect regressions if deg
gets stripped
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.
LGTM
There might be potential futur refactors seeing the similarities between the 2 rules.
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.
LGTM, thank you!
(The rule falls neatly into the wish of supporting modern CSS in the 16.x
)
Closes #7359
No, it's self explanatory.