fix: rtl display issue in author merge interface#11412
fix: rtl display issue in author merge interface#11412akramcodez wants to merge 1 commit intointernetarchive:masterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds right-to-left (RTL) text support for book titles in the author merge interface to ensure proper display of titles in RTL languages like Arabic and Hebrew.
- Introduces a new
is_rtl()utility function to detect RTL characters using Unicode bidirectional properties - Updates the author merge template to conditionally apply
dir="rtl"attribute to book title links when they contain RTL characters - Wraps edition count and publication year information in
dir="ltr"to maintain left-to-right display for numeric data
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openlibrary/plugins/upstream/utils.py | Adds is_rtl() function to detect RTL text using Unicode bidirectional character properties |
| openlibrary/templates/merge/authors.html | Refactors book title display to apply RTL directionality and improves formatting of edition metadata |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
akramcodez
force-pushed
the
fix/rtl-display-author-merge-interface
branch
from
9912d55 to
a1939eb
Compare
Freso
left a comment
Freso
left a comment
There was a problem hiding this comment.
Thank you for tackling this! A few comments. :)
| title="$_('Open in a new window')" | ||
| $:cond(is_rtl(doc['title']), 'dir="rtl"', '') | ||
| >$doc['title']</a> | ||
| <span class="smaller" dir="ltr"> |
There was a problem hiding this comment.
Don’t hardcode ltr here. It may not be ltr, depending on what the user’s locale is set to.
| <span class="smaller" dir="ltr"> | |
| <span class="smaller"> |
The <span> won’t inherit the dir property from <a> since it’s not a child of <a>, so there should be no reason to set it directly here at all. (If anything, there should be a dir property on <html> or <body> depending on the set locale, but that’s outside of the scope of this PR.)
| <a href="$doc['key']" | ||
| target="new" | ||
| title="$_('Open in a new window')" | ||
| $:cond(is_rtl(doc['title']), 'dir="rtl"', '') |
There was a problem hiding this comment.
| $:cond(is_rtl(doc['title']), 'dir="rtl"', '') | |
| dir="$:cond(is_rtl(doc['title']), 'rtl', 'ltr')" |
There might be situations where someone is browsing the site in, say, Arabic, and it turns out there’s an issue with Latin-script/LTR titles. If we believe that it’s not rtl, it’s probably ltr, so let’s declare that.
If we really want to optimise the code to not include the property when not needed, we’ll need to compare with the set locale’s direction and only output if they differ… but that’s probably overengineering for now.
|
Also, why did you remove the |
akramcodez
force-pushed
the
fix/rtl-display-author-merge-interface
branch
from
f1a7f63 to
a404941
Compare
Sorry for the mistakes, I actually planned to fix them tomorrow. But thanks a lot for the detailed explanation! The test is still failing though, could you please suggest what I should do now? |
Are you sure the syntax is |
github-actions
bot
added
the
Needs: Response
|
@cdrini I’ve done my best, but I’m kind of stuck now. Please review the changes and let me know if there are any issues. |
3 participants
Closes #10115
Problem
When merging authors, RTL work titles (Arabic, Hebrew) cause edition counts and years to display incorrectly mixed with the title text.
Before:
مدرسة العذاب 1 edition, 2009(numbers appear inside RTL text)After: Title and numbers are visually separated with proper text direction
Solution
is_rtl()utility function using Unicode bidirectional propertiesdir="rtl"only to RTL titlesdir="ltr"spanTechnical Changes
@publicis_rtl()functionStakeholders: @Freso @cdrini @mekarpeles