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

Correct the direction of the Persian language text #1670

Open
wants to merge 10 commits into
base: stable
Choose a base branch
from

Conversation

Mazafard
Copy link
Contributor

Fix the issue: Linkify does not function correctly with RTL text, particularly for Persian texts. #1546 #1547

@davidmz
Copy link
Member

davidmz commented Feb 25, 2024

Thanks, @Mazafard, but I think we can achieve this in a simpler way. Could you look at #1671? The live client with this patch is deployed to https://freefeed-freefeed-react-client-preview-pr-1671.surge.sh/

@Mazafard
Copy link
Contributor Author

@davidmz Yeah, definitely. Thank you.

Your solution is quite clever and addresses many scenarios. 💯 I particularly appreciate how it handles links and hashtags in a neat and straightforward manner. However, it lacks a mode, requiring me to resort to regex. This is especially the case when the text contains both Persian and English words, and notably when the text doesn't begin with Persian words, like in this example

I've combined your PR with mine and made a slight adjustment before sending it back to you. 👍

@davidmz
Copy link
Member

davidmz commented Feb 26, 2024

We have an ambiguity here. From your point of view this text is mostly in Farsi, but from my point of view it's more in English, and should be left-aligned:)

But if we want a smarter definition, we need a smarter algorithm as well. In your variant, one Arabic character in a long English text would be enough to switch to RTL mode.

@Mazafard
Copy link
Contributor Author

@davidmz I'm on the same page as you. I'm currently developing a new approach.

@davidmz
Copy link
Member

davidmz commented Feb 28, 2024

I have a few conclusions after the today's public test:

  1. One-line texts should have a special alignment, not just shift right to the column edge.
  2. Multi-line/paragraph text should have multi-block structure (now it emulates via "br" tags), and each block should have dir="auto". So, if the block is RTL it will be right-aligned, otherwise left-aligned.
  3. The comment markup is a problem. It mixes text and controls (author name, 'more' menu...) on the same line, and in the case of RTL it displays ugly. Probably, we should move controls to the separate line in multi-line case.

@Mazafard
Copy link
Contributor Author

  1. One-line texts should have a special alignment, not just shift right to the column edge.

I'm not sure I fully understand the issue here. Could you provide a bit more detail or an example, please?

  1. Multi-line/paragraph text should have multi-block structure (now it emulates via "br" tags), and each block should have dir="auto". So, if the block is RTL it will be right-aligned, otherwise left-aligned.

For handling multi-line or paragraph text, I've introduced a check to identify 'br' tags within the code, converting them to 'span' tags. This allows each line to have its own direction, aligning with the adjustments you've outlined. I'm confident in the effectiveness of this approach and plan to submit a PR once my testing is complete.

  1. The comment markup is a problem. It mixes text and controls (author name, 'more' menu...) on the same line, and in the case of RTL it displays ugly. Probably, we should move controls to the separate line in multi-line case.

You're absolutely right about the comment markup issue. Moving the controls to their own line for multi-line comments is definitely a smart move, and something I've done in the past too. However, the point about the comment and like icons positioned on the left side merits further attention. Especially in cases of brief comments(RTL), this positioning could result in the icons awkwardly floating, lacking a visual anchor. It suggests that moving the icons closer to the main controls could provide a more cohesive alignment.

Screenshot 2024-02-28 at 22 50 32

@davidmz
Copy link
Member

davidmz commented Feb 29, 2024

One-line texts should have a special alignment, not just shift right to the column edge.

I mean, if we just align the short text to the right, there will be a large blank space between the author name/avatar and the text. I think, we should keep such blocks text (!) right-aligned, but the block itself should be shifted to the left (because all other site UI is still left-aligned). Here is an example:
rtl

For handling multi-line or paragraph text, I've introduced a check to identify 'br' tags within the code, converting them to 'span' tags. This allows each line to have its own direction, aligning with the adjustments you've outlined. I'm confident in the effectiveness of this approach and plan to submit a PR once my testing is complete.

It is not so easy. We also have a <spoiler> tags, that works across the line breaks. They are works with br's, but will not work with span's. I, actually, want to rewrite that code.

Especially in cases of brief comments(RTL), this positioning could result in the icons awkwardly floating, lacking a visual anchor. It suggests that moving the icons closer to the main controls could provide a more cohesive alignment.

Need to think about it, thank you.

@Mazafard
Copy link
Contributor Author

Mazafard commented Mar 2, 2024

I mean, if we just align the short text to the right, there will be a large blank space between the author name/avatar and the text. I think, we should keep such blocks text (!) right-aligned, but the block itself should be shifted to the left (because all other site UI is still left-aligned). Here is an example:

I'm not totally convinced that the third option works well.
308830440-438818e6-e978-4f86-9656-b35ea870387c

You know, sprucing up the feed might seem like a good idea at first glance, but when you really dive into the whole timeline scene, mixing long chunks of text in Persian or English, things start to look a bit off and cluttered. Kinda feels like we're slapping on a band-aid and hoping for the best, doesn't it? From what I've noticed, the layout doesn't do justice to everything, especially those shorter posts hugging the right side. How about we try spreading things more evenly across the board? Like, maybe we could nudge the menu up to the top right corner of each post.
Screenshot 2024-02-29 at 12 43 03
That could make the whole page look more balanced. Then, those short RTL posts wouldn't stick out like a sore thumb in the timeline. Sounds like a plan, right?

@MMDJafari
Copy link
Contributor

@Mazafard @davidmz
Hey guys!
Regarding the issue with extra gap appearing between the Persian text and other post elements, adding display: flex to your existing .rtl style should resolve the problem. Here's the updated style:

&.rtl { text-align: right; display: flex; }

I have tested this, and it correctly aligns the content as desired. Please find a screenshot attached demonstrating the proper rendering.

Let me know if this solution works for you or if you need any further assistance.

Screenshot 1403-01-25 at 13 09 28

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants