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

#21 <- Feat: Add config for px-to-rem conversion #22

Merged
merged 2 commits into from
Dec 1, 2023
Merged

Conversation

nmn
Copy link
Contributor

@nmn nmn commented Nov 30, 2023

Add a new config option to control the automatic conversion of px values for font-size to rem.

The default value is true for backwards compatibility and because it is an a11y best practice.

@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 Nov 30, 2023
Copy link

compressed-size: runtime library

Size change: 0.00 kB
Total size: 2.36 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./packages/stylex/lib/stylex.js 1.02 (3.53) 0.00 (0.00) 0.0% (0.0%)
./packages/stylex/lib/StyleXSheet.js 1.34 (3.41) 0.00 (0.00) 0.0% (0.0%)

Copy link

compressed-size: e2e bundles

Size change: 0.00 kB
Total size: 1119.56 kB

View unchanged
Filename: gzip (minify) kB size kB change % change
./apps/rollup-example/.build/bundle.js 1005.35 (9859.61) 0.00 (0.00) 0.0% (0.0%)
./apps/rollup-example/.build/stylex.css 114.22 (456.05) 0.00 (0.00) 0.0% (0.0%)

@nmn nmn requested review from necolas, nestorvanz and Jta26 November 30, 2023 23:20
@necolas
Copy link
Contributor

necolas commented Dec 1, 2023

The default value is true for backwards compatibility and because it is an a11y best practice.

I want to make the case for it being false by default outside of Meta:

  1. All modern browsers scale px-based font-size when zooming.
  2. There's no way to opt-out of true in the cases where px is required.
  3. People will not expect px to be transformed, and will probably get confused.

@nmn
Copy link
Contributor Author

nmn commented Dec 1, 2023

  1. All modern browsers scale px-based font-size when zooming.

The a11y usecase is to be able to change font size without zooming the entire UI.

2 & 3 are good points and sufficient to convince me.

I will add an additional PR to change the default.

@nmn nmn merged commit fcaf6ff into update-deps Dec 1, 2023
8 checks passed
@nmn nmn deleted the rem-config branch December 1, 2023 13:45
nmn added a commit that referenced this pull request Dec 1, 2023
* Chore: Update dependencies

* #21 <- Feat: Add config for px-to-rem conversion (#22)

* Feat: Add config for px-to-rem conversion

* Chore: Update website for new config options (#23)
nmn added a commit that referenced this pull request Dec 1, 2023
* Chore: More sane config options
* Chore: Update dependencies (#21)
* Feat: Add config for px-to-rem conversion (#22)
* Chore: Update website for new config options (#23)
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