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

fix(NcAppNavigation + NcUserBubble + NcRichContenteditable): RTL support #6455

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

DorraJaouad
Copy link
Contributor

@DorraJaouad DorraJaouad commented Jan 24, 2025

☑️ Resolves

  • Many other components will need to be adjusted as well later.

Another issue to keep in mind: NcRichContenteditable is used with dir="auto" in Talk and is attached to the div with "_input" class. While we need to make a space for the emoji picker, the spacing is not adjusted to the RTL direction and it remains on the left side. We probably need to take the padding logic (and others) out of the _input class and put it in the wrapper class rich-contenteditable to follow the implemented language direction.

🖼️ Screenshots

🏚️ Before 🏡 After
image image
image image

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

@DorraJaouad DorraJaouad added feature: app-navigation Related to the app-navigation component feature: userbubble Related to the userbubble component RTL Right-to-Left languages support labels Jan 24, 2025
@DorraJaouad DorraJaouad added this to the 8.23.0 milestone Jan 24, 2025
@DorraJaouad DorraJaouad self-assigned this Jan 24, 2025
@Antreesy
Copy link
Contributor

Antreesy commented Jan 24, 2025

Another issue to keep in mind: NcRichContenteditable is used with dir="auto" in Talk and is attached to the div with "_input" class. While we need to make a space for the emoji picker, the spacing is not adjusted to the RTL direction and it remains on the left side.

But... I did that... 1e5eadd
Just drop dir="auto"
image

We probably need to take the padding logic (and others) out of the _input class and put it in the wrapper class rich-contenteditable to follow the implemented language direction.

Not sure that's wise. _input is rich element, with hover effects. wrapper is just to position optional label

@Antreesy

This comment was marked as off-topic.

@DorraJaouad
Copy link
Contributor Author

DorraJaouad commented Jan 24, 2025

dir="auto"

It's better to be kept, as it's user input and it will be strange to write in the opposite direction. Also, other apps have auto in the messages input.

@Antreesy
Copy link
Contributor

Then it should be handled upstream only if NcRichContenteditable will have a slot for leading / trailing icon.
Currently it's hacked in Talk, and therefore should be handled by Talk. As was discussed privately, we can work around paddings with CSS

@ShGKme ShGKme added the bug Something isn't working label Jan 29, 2025
@DorraJaouad DorraJaouad force-pushed the fix/noid/bidi-support-talk branch from 0d69357 to ae7d50e Compare January 29, 2025 17:58
Signed-off-by: Dorra Jaouad <dorra.jaoued7@gmail.com>
@DorraJaouad DorraJaouad force-pushed the fix/noid/bidi-support-talk branch from ae7d50e to ebad174 Compare January 30, 2025 18:37
@ShGKme ShGKme changed the title fix(bidi): apply logical properties for components used in Talk fix(NcAppNavigation + NcUserBubble + NcRichContenteditable): RTL support Jan 31, 2025
@ShGKme
Copy link
Contributor

ShGKme commented Jan 31, 2025

/backport to next

@DorraJaouad DorraJaouad merged commit 8091da7 into master Jan 31, 2025
23 checks passed
@DorraJaouad DorraJaouad deleted the fix/noid/bidi-support-talk branch January 31, 2025 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feature: app-navigation Related to the app-navigation component feature: userbubble Related to the userbubble component RTL Right-to-Left languages support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants