-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Layout corrections for RTL language environments, including completely flipped Grapher #2391
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
base: main
Are you sure you want to change the base?
Conversation
@HuanglinXu21 The change looks good to me. Could you please help follow up this PR and verify if the fix is valid or not? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pipeline of Main branch is not working properly right now. It should be an SDK version issue. We will fix it later. |
Hi @Avid29, the change looks good and we've tested that it's working. Now, it seems the pipeline is broken due to the unmatched SDK version in the pool's machine. I'll take some time to handle it. After that, we can proceed on your PR. Could you wait for a while until we fix the pipeline? |
@tian-lt Of course! Just lmk if/when you need any action on my part, and I'll get to it ASAP. |
Oh, I just realized that the numpad is backwards on the graph page as well. Conveniently, I can test that myself. It is currently not mirrored on other pages such as the Regular or Programmer page. I will explore how it's handled here, and add it to this branch, unless you believe this is best served as another PR. |
I've also changed the I know this is more natural for Hebrew speakers who write mathematical equations left to right, and I confirmed it's the same for arabic speakers by finding a math lecture in Arabic on YouTube. I've also found a video of a Hebrew math lecture, so you don't have to take my word for it. Hebrew math lecture ![]() |
@microsoft-github-policy-service agree |
</data> | ||
<data name="MinTextBlock.Text" xml:space="preserve"> | ||
<value>Min</value> | ||
<value>מינימום</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for correcting the translation.
However, these strings are generated by an internal automation tool. We need to report the translation error to the automation team so that they can fix the bot.
Could you please revert the changes of this file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I'll report this translation issue to the automation team and they will evaluate whether or not this is an translation error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, yes. They're both equally valid translations, but the text is next to a TextBox and which side it should be on depends on how that's written. I was playing with automating the flow direction based on the character set but decided that was a bit ridiculous. I forgot to revert the localization file changes afterwards 😬.
Since I've seen this now, I'm going to give another read over to make sure I didn't miss anything else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes Graph View layout issues in right-to-left (RTL) language environments by explicitly setting FlowDirection="LeftToRight"
on graph-related controls to prevent backwards rendering.
- Adds explicit
FlowDirection="LeftToRight"
to graph controls to override RTL inheritance - Restructures GraphingNumPad.xaml layout by moving grid definitions into a nested container
- Updates Hebrew localization for Min/Max labels from English to Hebrew text
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
src/Calculator/Views/GraphingCalculator/GraphingCalculator.xaml | Sets LeftToRight flow direction on the main Grapher control |
src/Calculator/Views/GraphingCalculator/EquationInputArea.xaml | Forces LeftToRight flow direction and left alignment for equation variable containers |
src/Calculator/Views/GraphingCalculator/GraphingNumPad.xaml | Restructures layout with nested grid and explicit LeftToRight flow direction |
src/Calculator/Resources/he-IL/Resources.resw | Updates Hebrew translations for Min/Max labels and removes BOM character |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
IsTabStop="false"/> | ||
|
||
<controls:CalculatorButton x:Name="AbsButton" | ||
<!-- Is there a reason the usual NumberPad is not used here--> |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] This comment appears to be a developer question rather than proper documentation. Consider either removing it or converting it to a proper TODO/FIXME comment with context about what should be investigated.
<!-- Is there a reason the usual NumberPad is not used here--> | |
<!-- TODO: Investigate why the usual NumberPad is not used here. Is there a specific reason for this implementation? --> |
Copilot uses AI. Check for mistakes.
</data> | ||
<data name="MinTextBlock.Text" xml:space="preserve"> | ||
<value>Min</value> | ||
<value>מינימום</value> |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The Hebrew translations for Min/Max labels should include comments explaining the translation choices, similar to other entries in the resource file that have descriptive comments.
Copilot uses AI. Check for mistakes.
</data> | ||
<data name="MaxTextBlock.Text" xml:space="preserve"> | ||
<value>Max</value> | ||
<value>מקסימום</value> |
Copilot
AI
Oct 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The Hebrew translations for Min/Max labels should include comments explaining the translation choices, similar to other entries in the resource file that have descriptive comments.
Copilot uses AI. Check for mistakes.
Hi @Avid29, we've fixed the GH Action problem at the main branch. Could you merge your PR branch with the main so that we can make the PR check pass? |
@tian-lt Done 😊 |
@@ -1,4 +1,4 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like there's still some change. (Maybe it's end-of-line. Could you revert it back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sorry. I'd reverted it by hand, so the encoding got changed still. Now I've just copied the file back from main and this should be fully resolved.
The changes look good to me. @Avid29 BTW, I found that the arrow button that scrolls the function bar looks not good. Thought it's also a bug in the main branch, it will be great if it can be fixed by this PR easily. @HuanglinXu21 Do you have any comments? |
@tian-lt I will gladly take a look! I was focusing on the Graph section since that's what I had named the PR for, but if I take care of that I can also flip the operator panels in other views and give the full app a look over for small oddities like that. |
Fixes #1352.
This PR is a reopening of #1812, against the
main
branch instead of the outdatedmaster
branch, with approval from @tian-ltDescription of the changes:
This change is created because the graph renders backwards when the app
FlowDirection
isRightToLeft
.This change overrides the
FlowDirection
for theGrapher
control, fixing that bugHow changes were validated:
Grapher
is close source.