Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Correct text alignment in RTL languages #9026

Closed
wants to merge 4 commits into from
Closed

Correct text alignment in RTL languages #9026

wants to merge 4 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Jul 8, 2022

For element-hq/element-web#14520 and element-hq/element-web#4771

This PR intends to relieve frustration of RTL language users.
cf. #5453

Please mind the position of (edited).
cf. #5453 (comment)

I modified the declarations for blockquote, ul, and ol to apply logical properties. Since I am not able to read RTL languages, it is not really possible for me to test edge cases though.

The ideal solution would be to implement logical properties all over the UI, but the task is too large for this PR. Therefore adjusting position of UI elements such as timestamp is out of scope.

Before After
IRC layout before1 Untitled2
Modern layout before2 Untitled
Bubble layout before3 Untitled3

type: defect


Here's what your changelog entry will look like:

🐛 Bug Fixes

  • Correct text alignment in RTL languages (#9026). Contributed by @luixxiul.

@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Jul 8, 2022
@github-actions github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Jul 8, 2022
@luixxiul luixxiul marked this pull request as ready for review July 8, 2022 14:17
@luixxiul luixxiul requested a review from a team as a code owner July 8, 2022 14:17
@t3chguy t3chguy requested a review from a team July 8, 2022 14:20
@luixxiul
Copy link
Contributor Author

luixxiul commented Jul 8, 2022

Though I don't find a regression, I think it would be better to merge this PR after some feedback comments are provided from native RTL language speakers.

I've posted a link to the netlify build on #5453 (comment).

Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

we've had massive issues with this sort of change in the past - I'd very much like to see screenshot tests of the timeline using various sample messages to ensure we didn't break anything.

#element-dev:matrix.org will likely be best to talk more about those sorts of tests.

Comment on lines 493 to 494
ol,
ul {
Copy link
Member

Choose a reason for hiding this comment

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

we should add comments for what this is targeting - it's not immediately obvious what the ol and ul actually are.

@luixxiul

This comment was marked as outdated.

@luixxiul luixxiul marked this pull request as draft July 9, 2022 02:06
@luixxiul
Copy link
Contributor Author

luixxiul commented Jul 9, 2022

converting to draft for now- looking for a way of implementing visual tests.

@turt2live
Copy link
Member

after the weekend the team should be able to help with tests in https://matrix.to/#/#element-dev:matrix.org

- Apply logical properties for ol, ul, and blockquote

Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
Signed-off-by: Suguru Hirahara <luixxiul@users.noreply.github.com>
@luixxiul
Copy link
Contributor Author

luixxiul commented Jun 4, 2023

Closed as RTL design will never land at least until Element X

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants