ItsJonQ / g2

✨ An experimental reimagining of WordPress components
http://g2-components.com/
MIT License
105 stars 12 forks source link

Use start and end rather than left and right #285

Closed sarayourfriend closed 3 years ago

sarayourfriend commented 3 years ago

Inspired by Chakra UI's RTL aware style props, I've drafted up this PR to try to do the same with ui helper functions: https://chakra-ui.com/docs/features/rtl-support

Basically instead of referring to left or right you refer to start and end. We can do this through the ui API that G2 exposes and have ui.margin.start as a function which will return either margin-left or margin-right depending on the RTL status.

// when RTL
ui.margin.start('5px') => 'margin-right: 5px;'

// when LTR
ui.margin.start('5px') => 'margin-left: 5px;'

Also... this is a massive PR, but I'm not sure how else to do it.

This can just be a stop-gap solution before we find a way to do automatic processing of RTL styles. Maybe we should invest in that instead of this? What do you think?

vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/itsjonq/g2/K5wTuiQVjHGYBFpzu44YxkYqAVQB
✅ Preview: Failed

sarayourfriend commented 3 years ago

I guess we could introduce the new API without making all these changes, but the API differences are significant enough that it's still difficult to do so.

sarayourfriend commented 3 years ago

I'm revisiting this PR today, I'm going to try to find a different approach that doesn't involve making all the changes. Hopefully we can make the changes here and then just update the already integrated components rather than duplicating our changes everywhere.

One question I have... is it necessary to make the styles "responsive"? That is... do we need to check the RTL status at runtime or can we do it on page load, once, and then define the presets based on that?

For example, do we need to do:

border.start = () => getRTL() ? rtlStyle : ltrStyle;

or can we do

border.start = getRtl() ? rtlStyle : ltrStyle;

It'd be ideal if we can just do the latter.

ItsJonQ commented 3 years ago

@sarayourfriend Haiiiiiii!!


automatic processing of RTL styles. Maybe we should invest in that instead of this?

I'm always in favour of automation when possible 😊


This is an ambitious PR indeed! I remember seeing the Chakra update a while back and feeling intrigued.

Looking at the code, my immediate reaction was that it didn't feel intuitive to use start/end in place of left and right - especially if I need to escape a string to use the ui. mixin.

1 part is due to unfamiliarity, which I'm sure I can overcome :). The other part is that the properties aren't native CSS values.

There are some that are close, like ui.padding.start is like padding-inline-start.


One question I have... is it necessary to make the styles "responsive"?

I'm not sure if it's 100% necessary. I made that change recently an in attempt to resolve another RTL related issue.

I think we're (by we, I mean the JS community as a whole) is still trying to figure out how to elegantly gracefully handle RTL with CSS-in-JS.

The CacheProvider + stylis-plugin-rtl strategy for libraries like Chakra is the closest I've seen.

It requires a full page reload though:

    // force a reload for it to work correctly.
    reload()

Maybe that's okay. However, I think native .css based strategies can handle the swapping without page reloads.

sarayourfriend commented 3 years ago

This was an interesting concept I think we should still explore but I'm going to close this PR for now. After we merge the PR to integrate the styles package and as we add back the presets and mixins we can evaluate how best to handle RTL styles.