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

style: Add RTL support #643

Closed
wants to merge 2 commits into from
Closed

style: Add RTL support #643

wants to merge 2 commits into from

Conversation

GideonMax
Copy link

@GideonMax GideonMax commented Jul 26, 2023

switched directional attributes like:
margin-left/right
padding-left/right
with
margin-inline-start/end
padding-inline-start/end
which switch direction when in rtl mode

What does it do?

css now works as expected in both rtl and ltr pages

Fixes # (issue)

Fixes #642

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • Added tests that prove my fix is effective or that my feature works (my fix is purely visual, not really testable)
  • New and existing unit tests pass locally with my changes
  • My changes generate no new warnings

switched directional attributes like:
margin-*
padding-*
with
margin-inline-*
padding-inline-*

Added direction option to Options story
inline-start/end might be a bit confusing so I commented what each of them means in ltr
@mrchief
Copy link
Collaborator

mrchief commented Aug 1, 2023

This is pretty cool, thanks for sending this! Ava JS is messing up the tests so I need to run some offline checks to make sure everything checks out, stay tuned!

@GideonMax
Copy link
Author

GideonMax commented Aug 6, 2023

@mrchief after looking a bit more into it
the -inline- properties in css automatically get auto vendor prefixing, but the one in the style tag (for indentation) does not.
So I just wanted to ask which fix you would prefer (in terms of code style):
a. use a class + css variable so that the style is auto prefixed
b. manually add vendor prefixes in the style tag

@mrchief
Copy link
Collaborator

mrchief commented Aug 7, 2023

@mrchief after looking a bit more into it the -inline- properties in css automatically get auto vendor prefixing, but the one in the style tag (for indentation) does not. So I just wanted to ask which fix you would prefer (in terms of code style): a. use a class + css variable so that the style is auto prefixed b. manually add vendor prefixes in the style tag

With solution a, do you mean adding a autoprefixer to our own build process so that the styles are auto prefixed or is it something the caller would be have to use? If it's the former, I think that is ok. The later imposes an additional build step for the caller which I'd like to avoid.

@GideonMax
Copy link
Author

@mrchief after looking a bit more into it the -inline- properties in css automatically get auto vendor prefixing, but the one in the style tag (for indentation) does not. So I just wanted to ask which fix you would prefer (in terms of code style): a. use a class + css variable so that the style is auto prefixed b. manually add vendor prefixes in the style tag

With solution a, do you mean adding a autoprefixer to our own build process so that the styles are auto prefixed or is it something the caller would be have to use? If it's the former, I think that is ok. The later imposes an additional build step for the caller which I'd like to avoid.

Your build process already has autoprefixing.
if you look at the build output,
margin-inline-start: 2px; got transformed into -webkit-margin-start:2px;margin-inline-start:2px

@mrchief
Copy link
Collaborator

mrchief commented Aug 9, 2023

Your build process already has autoprefixing. if you look at the build output, margin-inline-start: 2px; got transformed into -webkit-margin-start:2px;margin-inline-start:2px

Right, but your idea made it sound like we need additional stuff.

@GideonMax
Copy link
Author

Your build process already has autoprefixing. if you look at the build output, margin-inline-start: 2px; got transformed into -webkit-margin-start:2px;margin-inline-start:2px

Right, but your idea made it sound like we need additional stuff.

well, we do not need anything extra, and the idea also does not add any requirements for the end developer/caller, this is purely a code style thing

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 24, 2023
@mrchief
Copy link
Collaborator

mrchief commented Sep 25, 2023

Not stale

@stale stale bot removed the stale label Sep 25, 2023
Copy link

github-actions bot commented Nov 9, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 10 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 9, 2023
@github-actions github-actions bot closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rtl support
2 participants