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

feat: add autofix for logical to inline properties in valid-styles rule #805

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

mellyeliu
Copy link
Member

@mellyeliu mellyeliu commented Dec 10, 2024

Context

As per internal discussions, let's add an autofix into the valid-styles rule. Some of this logic exists in the valid-shorthands rule, but it makes sense to move the erroring here for the repos in which valid-shorthands is not enabled.

I've kept the descriptor in the autofix short and simple to avoid clutter on a lint error that will likely crop up a lot, but can also link to https://fb.workplace.com/groups/stylexfyi/posts/1769833193785624 or something related as context. Open to feedback here!

As a next step, we should work towards splitting up the valid-styles rule into more discrete chunks to increase the granularity of error reporting/messaging (perhaps after the codemodding)

Tests

Added tests for the following styles:

  • marginStart -> marginInlineStart
  • marginEnd -> marginInlineEnd
  • marginHorizontal -> marginInline
  • marginVertical -> marginBlock
  • paddingStart -> paddingInlineStart
  • paddingEnd -> paddingInlineEnd
  • paddingHorizontal -> paddingInline
  • paddingVertical -> paddingBlock

Pre-flight checklist

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 10, 2024
Copy link

github-actions bot commented Dec 10, 2024

workflow: benchmarks/size

Comparison of minified (terser) and compressed (brotli) size results, measured in bytes. Smaller is better.

@stylexjs/[email protected] size:compare
./size-compare.js /tmp/tmp.Rkrpvaj0ih /tmp/tmp.ozVDIZjUuX

Results Base Patch Ratio
stylex/lib/stylex.js
· compressed 729 729 1.00
· minified 2,541 2,541 1.00
stylex/lib/StyleXSheet.js
· compressed 1,266 1,266 1.00
· minified 3,776 3,776 1.00
rollup-example/.build/bundle.js
· compressed 562,983 562,983 1.00
· minified 10,185,368 10,185,368 1.00
rollup-example/.build/stylex.css
· compressed 99,229 99,229 1.00
· minified 745,845 745,845 1.00

@mellyeliu mellyeliu changed the title feat: add autofix to inline properties in valid-styles rule feat: add autofix logical to inline properties in valid-styles rule Dec 10, 2024
@mellyeliu mellyeliu requested review from necolas and nmn and removed request for necolas December 10, 2024 22:17
@mellyeliu mellyeliu changed the title feat: add autofix logical to inline properties in valid-styles rule feat: add autofix for logical to inline properties in valid-styles rule Dec 10, 2024
errors: [
{
message:
'The key "marginStart" is not allowed by stylex. Use "marginInlineStart" instead.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like this:

"The key 'marginStart' is not a standard CSS property. Did you mean 'marginInlineStart'?"

Communicating that it's non-standard is useful context for people

Copy link
Contributor

@necolas necolas left a comment

Choose a reason for hiding this comment

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

LGTM (once message bikeshedding is complete)

@mellyeliu
Copy link
Member Author

LGTM (once message bikeshedding is complete)

I had to google this #til Parkinson's Law of Triviality

@mellyeliu mellyeliu merged commit f9ec2b4 into main Dec 10, 2024
8 checks passed
@mellyeliu mellyeliu deleted the valid-styles-fix branch December 10, 2024 22:48
aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
aminaopio pushed a commit to aminaopio/stylex that referenced this pull request Dec 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants