Dark Mode

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Add RTL support#51376

Merged
eladkal merged 4 commits intoapache:mainfrom
guan404ming:support-rtl
Jun 9, 2025
Merged

Add RTL support#51376
eladkal merged 4 commits intoapache:mainfrom
guan404ming:support-rtl

Conversation

Copy link
Member

guan404ming commented Jun 3, 2025 *
edited
Loading

Related Issue

#51187
cc @bbovenzi @jscheffl @eladkal

Why

  • to support rtl lang we need to add "dir" attribute in

How

  • add it dynamically using i18n-next api hook
  • determine the dir using i18n.dir()
  • add LocaleProvider align to official tutorial for chakra-ui
Screen.Recording.2025-06-04.at.1.07.49.AM.mov

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

RoyLee1224 and kevinhongzl reacted with thumbs up emoji
guan404ming requested review from bbovenzi, jscheffl, pierrejeambrun, ryanahamilton and shubhamraj-git as code owners June 3, 2025 17:17
boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Jun 3, 2025
guan404ming changed the title Add rtl support Add RTL support Jun 3, 2025
jscheffl approved these changes Jun 3, 2025
Copy link
Contributor

jscheffl left a comment

Choose a reason for hiding this comment

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

Cool! That looks simple. I thought it is a bit more code needed.

Still I am a bit puzzled that the navbar does not go on the right side, would expect this for consistency. But would leave the vote to some native reader to feedback

guan404ming reacted with thumbs up emoji
pierrejeambrun mentioned this pull request Jun 4, 2025
pierrejeambrun added this to the Airflow 3.1.0 milestone Jun 4, 2025
pierrejeambrun mentioned this pull request Jun 4, 2025
3 tasks
Copy link
Member

pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Nice.

I also think the navbar should swap right, a simple flex box reverse should do the trick in the base layout.

Also some input have minor issues:

Copy link
Contributor

bbovenzi commented Jun 4, 2025

I would like to defer to users like @Dev-iL if the would prefer the nav bar to be moved to the right side too.
Dev-iL reacted with eyes emoji

Copy link
Contributor

eladkal commented Jun 4, 2025

Thanks for raising this!
I will test this change combined with the Hebrew translation tommorow
guan404ming reacted with rocket emoji

Copy link
Member Author

guan404ming commented Jun 5, 2025

Personally, I hate it when GUIs are mirrored and I stay away from HE software for that reason alone.

I've noticed the issue of the nav bar and I also saw some comments above. Thus, I think we should listen to native speakers' opinions to decide whether we should also mirror it.

Copy link
Member Author

guan404ming commented Jun 5, 2025

Also some input have minor issues:

@pierrejeambrun I couldn't find or reproduce this issue on my local machine. Could you tell me more about the detail for this issue? I could diagnose and fix it. Thanks!

Copy link
Member

pierrejeambrun left a comment

Choose a reason for hiding this comment

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

In my picture above, some inputs do not have the arrow down icon that is mirrored properly.

Some of them do, but some don't and therefore the text is overlapping with the carret down.

Let me know if you also experience this.

Copy link
Member Author

guan404ming commented Jun 5, 2025 *
edited
Loading

In my picture above, some inputs do not have the arrow down icon that is mirrored properly.

Some of them do, but some don't and therefore the text is overlapping with the carret down.

Let me know if you also experience this.

I got it but it seems really strange. It would not overlap when I opened it with Chrome but it would be broken when I using Arc Browser to open it. The arrow down icon is not mirrored properly for both them. Diagnosing it currently.

Chrome

Arc

Copy link
Collaborator

Dev-iL commented Jun 5, 2025 *
edited
Loading

Chrome

Did you notice how "MetaDatabase" has the checkmark on the opposite side of the other tags?

Also, in the 2nd chrome screenshot each deopdown seems to be mirrored differently.
guan404ming reacted with thumbs up emoji

Copy link
Contributor

eladkal commented Jun 6, 2025

I reviewed several pages and it looks good.

Most of the things that don't work properly are due to translations issues so I will address them
There is one thing with strings that start with numbers they are not aligned properly. The number should be first.
guan404ming reacted with thumbs up emoji

eladkal added the type:new-feature Changelog: New Features label Jun 7, 2025
guan404ming force-pushed the support-rtl branch from 0ec769d to dbc38bc Compare June 9, 2025 08:50
Copy link
Contributor

jscheffl commented Jun 9, 2025

I reviewed several pages and it looks good.

Hi @eladkal Thanks for the review. I was missing a (more) clear statement if switching to RTL means that navbar also need to switch or if it is rather "better"/okay to keep it left. For me it is a bit tstrange to switch all to RTL but keeping the navbar left. But I am not an expert in this topic.

...or @shahar1 do you have an opinion?
guan404ming reacted with heart emoji

Copy link
Member Author

guan404ming commented Jun 9, 2025 *
edited
Loading

Hi @eladkal @Dev-iL, after my investigation, the select issue is fixed by manual add style for RTL on IndicatorGroup which seems like the chakra-ui issue and I could help open a fix pr to chakra-ui.

I also upgrade our chakra-ui package from 3.17.0 to 3.20.0 version since it has some rtl fixes which would reduce the manual fix above.

Please take another look and let me know if there is anything need modifications, thanks!

Current ui looks like

Screen.Recording.2025-06-09.at.4.39.53.PM.mov

Copy link
Contributor

shahar1 commented Jun 9, 2025

I reviewed several pages and it looks good.

Hi @eladkal Thanks for the review. I was missing a (more) clear statement if switching to RTL means that navbar also need to switch or if it is rather "better"/okay to keep it left. For me it is a bit tstrange to switch all to RTL but keeping the navbar left. But I am not an expert in this topic.

...or @shahar1 do you have an opinion?

In general, I think that the menu should be on the right - that's how it works in Wikipedia, for example.
Is it complicated to implement?
guan404ming and bbovenzi reacted with thumbs up emoji

Copy link
Contributor

eladkal commented Jun 9, 2025

In general, I think that the menu should be on the right - that's how it works in Wikipedia, for example. Is it complicated to implement?

I agree
guan404ming and bbovenzi reacted with thumbs up emoji

guan404ming force-pushed the support-rtl branch from dbc38bc to ba2434f Compare June 9, 2025 10:16
Copy link
Member Author

guan404ming commented Jun 9, 2025

In general, I think that the menu should be on the right - that's how it works in Wikipedia, for example.
Is it complicated to implement?

Sure, that makes sense. it's not complex to implement. It would look like

Copy link
Collaborator

Dev-iL commented Jun 9, 2025

In general, I think that the menu should be on the right - that's how it works in Wikipedia, for example. Is it complicated to implement?

No objections here, it is the known convention after all.

Copy link
Collaborator

Dev-iL commented Jun 9, 2025 *
edited
Loading

Sure, that makes sense. it's not complex to implement. It would look like

You know, I actually prefer this for the English version. Any chance we can make the navbar side user-configurable and not language-specific?

Copy link
Member Author

guan404ming commented Jun 9, 2025

You know, I actually prefer this for the English version. Any chance we can make the navbar side user-configurable and not language-specific?

Maybe we could open another issue for it to make sure if there are many people love this feature as well. I think it is not complex to implement but this change needs community's support~
Dev-iL reacted with thumbs up emoji

eladkal approved these changes Jun 9, 2025
Copy link
Contributor

eladkal left a comment

Choose a reason for hiding this comment

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

LGTM

There are more stuff we need to fix but this is a good start. Lets handle them with improvment PRs.

eladkal merged commit d52ce42 into apache:main Jun 9, 2025
43 checks passed
eladkal mentioned this pull request Jun 9, 2025
1 task
guan404ming mentioned this pull request Jun 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

pierrejeambrun pierrejeambrun left review comments

eladkal eladkal approved these changes

jscheffl jscheffl approved these changes

bbovenzi Awaiting requested review from bbovenzi bbovenzi is a code owner

ryanahamilton Awaiting requested review from ryanahamilton ryanahamilton is a code owner

shubhamraj-git Awaiting requested review from shubhamraj-git shubhamraj-git is a code owner

Assignees

No one assigned

Labels

area:UI Related to UI/UX. For Frontend Developers. type:new-feature Changelog: New Features

Projects

None yet

Milestone

Airflow 3.1.0

Development

Successfully merging this pull request may close these issues.

7 participants