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

Update text alignment for better rtl support #319

Closed

Conversation

hichemfantar
Copy link
Contributor

Change text alignment properties to improve consistency across navbar and pagination components.

@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 Jan 14, 2025
Copy link

netlify bot commented Jan 14, 2025

Deploy Preview for infima ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b2654b4
🔍 Latest deploy log https://app.netlify.com/sites/infima/deploys/6786579eb84e1a0008f92fe3
😎 Deploy Preview https://deploy-preview-319--infima.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@slorber
Copy link
Collaborator

slorber commented Jan 16, 2025

This change feels harmless but also not particularly useful considering our existing postcss rtl plugin already flips all non-logical properties

Does it have any effect in practice?
Demo: https://deploy-preview-319--infima.netlify.app/demo/

How does this improve consistency? Afaik we are almost not using logical properties anywhere so I'd say it makes things less consistent by introducing logical properties but only in a single place?

An example with the navbar

  margin: 0 0 0.2rem -0.5rem;
  padding: 0.6rem 1.5rem;
  text-align: left;
  top: -0.5rem;

If we want consistency + logical properties, it becomes:

  margin-block-start: 0;
  margin-block-end: 0.2rem;
  margin-inline-start: -0.5rem;
  margin-inline-end: 0;
  padding-block: 0.6rem;
  padding-inline: 1.5rem;
  text-align: start;
  inset-block-start: -0.5rem;

But then, CSS is heavier than if we just flipped everything with our PostCSS plugin.

@hichemfantar
Copy link
Contributor Author

Alright, it makes sense to only rely on the plugin to do this. closing

@hichemfantar hichemfantar deleted the better-rtl-support branch January 16, 2025 12:36
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