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.

Sign up for GitHub

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

Jump to bottom

fix: Remove non-standard CSS properties #605

Merged
merged 3 commits into from
Jun 23, 2024
Merged

fix: Remove non-standard CSS properties #605

merged 3 commits into from
Jun 23, 2024

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Jun 14, 2024

Fixes #604

@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 Jun 14, 2024
@github-actions GitHub Actions
Copy link

compressed-size: runtime library

Size change: 0.00 kB
Total size: 2.52 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./packages/stylex/lib/stylex.js 1.04 (3.22) 0.00 (0.00) 0.0% (0.0%)
./packages/stylex/lib/StyleXSheet.js 1.48 (3.75) 0.00 (0.00) 0.0% (0.0%)

@github-actions GitHub Actions
Copy link

compressed-size: e2e bundles

Size change: 0.00 kB
Total size: 1128.74 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./apps/rollup-example/.build/bundle.js 1005.32 (10185.35) 0.00 (0.00) 0.0% (0.0%)
./apps/rollup-example/.build/stylex.css 123.42 (774.34) 0.00 (0.00) 0.0% (0.0%)

necolas
necolas reviewed Jun 14, 2024
View reviewed changes
packages/eslint-plugin/src/stylex-valid-styles.js
@@ -592,20 +592,9 @@ const borderImage = makeUnionRule(
isString,
borderImageRepeat,
);
// const borderInlineEnd = makeUnionRule(borderWidth, borderStyle, color);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these already declared elsewhere?

Copy link
Contributor Author

@nmn nmn Jun 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Border shorthands are intentionally disallowed. You gotta use borderInlineEndWidth, Style and Color instead.

Also, there were originally lots of redundant declarations which I've been pruning whenever I make edits.

After adding much stricter validation for StyleX, I will overhaul the CSS types. In the meantime, I'm trying to migrate away from using strings for CSS variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yes, sorry I misread those

packages/eslint-plugin/src/stylex-valid-styles.js
const borderLeftColor = color;
const borderLeftStyle = brStyle;
const borderLeftWidth = borderWidth;
const borderRightColor = color;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these meant to be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, borderRightColor references color directly instead.

@nmn nmn merged commit 3b0d4b8 into main Jun 23, 2024
9 checks passed
@nmn nmn deleted the fix/remove-non-standard branch June 23, 2024 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@necolas necolas necolas left review comments

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove non-standard CSS properties from types
3 participants
@nmn @necolas @facebook-github-bot