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

[CLNP-3674] feat: RTL(Right To Left) display support #1132

Merged
merged 5 commits into from
Jun 27, 2024
Merged

Conversation

AhyoungRyu
Copy link
Contributor

@AhyoungRyu AhyoungRyu commented Jun 11, 2024

Addresses https://sendbird.atlassian.net/browse/CLNP-3543

I added htmlTextDirection prop(The original element attribute name is dir but I chose to be more elaborate in our app) to SendbirdProvider to support Right To Left(RTL) text direction display for the middle east customers.
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/dir

Copy link

netlify bot commented Jun 11, 2024

Deploy Preview for sendbird-uikit-react ready!

Name Link
🔨 Latest commit b413fc6
🔍 Latest deploy log https://app.netlify.com/sites/sendbird-uikit-react/deploys/667a13f6e2b7da0008ebf335
😎 Deploy Preview https://deploy-preview-1132--sendbird-uikit-react.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.

@@ -136,6 +137,12 @@ const meta: Meta<typeof App> = {
'A property that determines whether to automatically select another channel when the currently selected channel is deleted, or the user exits the channel, causing it to be deselected in the channel list.',
control: 'boolean',
},
htmlTextDirection: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-06-11 at 4 06 45 PM
Added this one too for the easier test :)

@AhyoungRyu AhyoungRyu requested review from bang9, HoonBaek and liamcho and removed request for bang9 and HoonBaek June 11, 2024 07:24
@AhyoungRyu AhyoungRyu self-assigned this Jun 11, 2024
@AhyoungRyu AhyoungRyu requested a review from chrisallo June 11, 2024 07:24
@AhyoungRyu AhyoungRyu changed the title feat: add htmlTextDirection prop to SendbirdProvider [CLNP-3674] feat: add htmlTextDirection prop to SendbirdProvider Jun 11, 2024
@bang9
Copy link
Contributor

bang9 commented Jun 11, 2024

Could you generate a preview URL for testing?

Copy link
Contributor

@bang9 bang9 left a comment

Choose a reason for hiding this comment

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

Shouldn't we lower it to the module level instead of the app level?
And when customers customize and reuse components, we might need to lower the level even further for it to apply.

checked docs: https://sendbird.atlassian.net/wiki/spaces/SDK/pages/2401009678/RTL+Support

@AhyoungRyu AhyoungRyu force-pushed the feat/CLNP-3674 branch 2 times, most recently from 688dc8f to aa8308a Compare June 11, 2024 09:09
@AhyoungRyu
Copy link
Contributor Author

@bang9 Sure. Here the link -> https://deploy-preview-1132--sendbird-uikit-react.netlify.app/?htmlTextDirection=rtl

@bang9
Copy link
Contributor

bang9 commented Jun 11, 2024

One consideration we need to address is whether it is acceptable for this to be merged into the main branch without the layout being fixed. As you know, deployments can happen at any time on the main branch, and if that happens, this feature might be exposed to customers in an incomplete state.

@AhyoungRyu
Copy link
Contributor Author

AhyoungRyu commented Jun 12, 2024

@bang9 Oh I thought there's no one who uses this until we make this public 😄 Then I won't merge this one and keep working based on this branch.

Copy link
Collaborator

@chrisallo chrisallo left a comment

Choose a reason for hiding this comment

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

lgtm!

@AhyoungRyu
Copy link
Contributor Author

AhyoungRyu commented Jun 14, 2024

I think it's ready so I'm gonna deliver the preview link to the design & QA team. But I'll merge this after finishing the QA process.

@AhyoungRyu AhyoungRyu changed the title [CLNP-3674] feat: add htmlTextDirection prop to SendbirdProvider [CLNP-3674] feat: RTL(Right To Left) display support Jun 18, 2024
@AhyoungRyu AhyoungRyu changed the title [CLNP-3674] feat: RTL(Right To Left) display support [CLNP-3674][DO NOT MERGE] feat: RTL(Right To Left) display support Jun 18, 2024
@bang9 bang9 marked this pull request as draft June 18, 2024 02:30
@AhyoungRyu AhyoungRyu changed the title [CLNP-3674][DO NOT MERGE] feat: RTL(Right To Left) display support [CLNP-3674] feat: RTL(Right To Left) display support Jun 18, 2024
Applied [postcss-rtlcss
plugin](https://github.com/elchininet/postcss-rtlcss) for the better RTL
support without tedious manual CSS style modifications.

- [x] Added plugin configuration to storybook/main.ts - Allows testing
of RTL support during development.
- [x] Added plugin configuration to rollup.config.js - Ensures build
artifacts support RTL languages.
- [x] Created useHTMLTextDirection custom hook - Applies the dir
attribute to modal and dropdown portal components for correct RTL
rendering.

postcss-rtlcss automates the conversion of LTR CSS to RTL, ensuring
consistency and reducing manual errors.
It automates CSS transformation by converting LTR styles to RTL,
ensuring proper alignment and display in RTL mode.

You can play around in here https://elchininet.github.io/postcss-rtlcss/
to see what kind of modifications could be made by this plugin.
@AhyoungRyu AhyoungRyu marked this pull request as ready for review June 27, 2024 07:23
@AhyoungRyu
Copy link
Contributor Author

I got an approval from QA team so it seems okay to merge this into main branch 🎉
https://sendbird.slack.com/archives/C0794CS06EM/p1719386625273959

FYI, I got separate approvals for sub-PRs I made after I got 🆗 from Airen & Chris :)

@AhyoungRyu AhyoungRyu merged commit 3bf0a1d into main Jun 27, 2024
8 checks passed
@AhyoungRyu AhyoungRyu deleted the feat/CLNP-3674 branch June 27, 2024 07:28
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.

3 participants