RTL: force (%) symbol to follow text direction #978

ARMBouhali · 2022-09-15T16:18:47Z

TL;DR - This commit makes the percentage (%) symbol appear correctly after numbers for RTL languages.

Background

In RTL languages such as Arabic, the correct placement for % is to come after the number, following the writing direction.
For example, 25% in RTL should appear as %25 instead.

What changed?

We use 2 frontend-platform functions isRtl and getLocale to apply the fix only if the current user language is right-to-left.
To avoid using a space to enforce the % to follow the RTL direction, we use instead the right-to-left marker character which is a non-printing character that does the exact job.
This change is applied to the following components in the progress tab:
- Course completion donut chart: 'XX% complete' label
- Grade Bar: current grade & passing grade tooltips.
- Grade summary table

Screenshots
Faulty layout in RTL: the % sign appears before the value (to the right)

Fixed layout in RTL: the % sign appears correctly after the value (to the left)

openedx-webhooks · 2022-09-15T16:18:50Z

Thanks for the pull request, @ARMBouhali! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

natabene · 2022-09-19T15:57:53Z

@ARMBouhali Thank you for the contribution, we will review this once your agreement has been sorted out.

natabene · 2022-09-27T15:27:51Z

@ARMBouhali Great, your CLA check is green, our team can review now.

ARMBouhali · 2022-09-30T23:40:40Z

I've updated the commit message to comply with commitlint rules.

brian-smith-tcril

it feels like there's a lot of redundant code in here, but nothing pops to mind as a better way to handle RTL percentages. @arbrandes any chance you have thoughts on any other ways to handle this?

brian-smith-tcril · 2022-10-12T17:58:23Z

src/course-home/progress-tab/grades/course-grade/CurrentGradeTooltip.jsx

@@ -41,7 +41,7 @@ function CurrentGradeTooltip({ intl, tooltipClassName }) {
        overlay={(
          <Popover id={`${isPassing ? 'passing' : 'non-passing'}-grade-tooltip`} aria-hidden="true" className={tooltipClassName}>
            <Popover.Content data-testid="currentGradeTooltipContent" className={isPassing ? 'text-white' : 'text-dark-700'}>
-              {currentGrade.toFixed(0)}%
+              {currentGrade.toFixed(0)}{isLocaleRtl ? '\u200f' : ''}%


is there a specific reason to use the _ ? _ : _ pattern here instead of hte _ && _ pattern? it seems both patterns are used in this PR.

@brian-smith-tcril

is there a specific reason to use the _ ? _ : _ pattern here instead of hte _ && _ pattern? it seems both patterns are used in this PR.

indeed there is a reason; it was for a clean rendering for each context.

condition && text was used to render a JSX element (DOM literal) as a shorthand for condition ? text : null.

condition ? text : '' was needed to render a string within a string template, otherwise, when condition is false, condition && text would give a 'false' string instead of an empty string ''

it feels like there's a lot of redundant code in here...

Concise ways to deal with this:

Add RTL function(s) to frontend-platform/i18n to do RTL operations. e.g i18n.adjust('%')

Render a translated percentage expression using intl.formatMessage or <FormattedMessage /> and let translators deal with each instance of % in every language as I did for the Arabic learning MFE strings. Transifex unfortunately does not show special chars so it's not possible to observe the result without exporting a translation file.

mphilbrick211 · 2022-10-12T20:59:45Z

@mattcarter - Hi Mat! Could you please allow actions / tests to run on this item?

natabene · 2022-10-13T01:00:10Z

here you go

codecov · 2022-10-13T01:06:20Z

Codecov Report

Base: 84.13% // Head: 84.16% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (bbd286b) compared to base (759d154).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #978      +/-   ##
==========================================
+ Coverage   84.13%   84.16%   +0.03%     
==========================================
  Files         264      264              
  Lines        4519     4529      +10     
  Branches     1158     1165       +7     
==========================================
+ Hits         3802     3812      +10     
  Misses        697      697              
  Partials       20       20

Impacted Files	Coverage Δ
...ess-tab/course-completion/CompletionDonutChart.jsx	`100.00% <100.00%> (ø)`
...ss-tab/grades/course-grade/CurrentGradeTooltip.jsx	`94.44% <100.00%> (+0.32%)`	⬆️
...ss-tab/grades/course-grade/PassingGradeTooltip.jsx	`92.30% <100.00%> (+0.64%)`	⬆️
...ess-tab/grades/grade-summary/GradeSummaryTable.jsx	`100.00% <100.00%> (ø)`
...b/grades/grade-summary/GradeSummaryTableFooter.jsx	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ARMBouhali · 2022-10-13T08:20:53Z

I've rebased the branch for Codecov tests to pass.

brian-smith-tcril

The ways to reduce redundancy mentioned in #978 (comment) feel out of scope for this PR (but it'd be great to make an issue for them)

I see no reason to wait for those changes before merging this.

LGTM!

mphilbrick211 · 2022-10-17T15:44:42Z

@mattcarter - Hi Mat! Could you please allow actions / tests to run on this item?

Friendly ping on this :)

ARMBouhali · 2022-10-24T13:11:07Z

@mattcarter @mphilbrick211 One last failing test, but it appears out of reach for me.
an error occurred with the coverage report upload:

Error: Codecov: Failed to properly upload: The process '/home/runner/work/_actions/codecov/codecov-action/v3/dist/codecov' failed with exit code 255

Can anyone check what's actually wrong?

arbrandes

These changes look good to me, and tests are passing. Approved.

Before merging, however, it would be good to have an ok from a frontend-app-learning dev. If none emits an opinion in a day or so, I'll press the merge button.

ARMBouhali · 2022-10-25T13:54:15Z

@arbrandes FYI a similar PR for frontend-app-gradebook has been approved and merged a while ago.

mphilbrick211 · 2022-10-26T01:16:52Z

These changes look good to me, and tests are passing. Approved.

Before merging, however, it would be good to have an ok from a frontend-app-learning dev. If none emits an opinion in a day or so, I'll press the merge button.

@mattcarter

openedx-webhooks · 2022-10-26T13:31:58Z

@ARMBouhali 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 15, 2022

natabene changed the title ~~RTL: force (%) symbol to follow text direction~~ RTL: force (%) symbol to follow text direction Sep 27, 2022

ARMBouhali force-pushed the rtl-fix-percentage-direction branch from 5d53aef to 7580a1b Compare September 30, 2022 23:33

brian-smith-tcril reviewed Oct 12, 2022

View reviewed changes

ARMBouhali force-pushed the rtl-fix-percentage-direction branch 3 times, most recently from 81faa69 to 4512639 Compare October 13, 2022 08:11

brian-smith-tcril approved these changes Oct 13, 2022

View reviewed changes

ARMBouhali force-pushed the rtl-fix-percentage-direction branch from 4512639 to 17e8f44 Compare October 24, 2022 11:40

fix(rtl): force (%) symbol to follow text direction

bbd286b

ARMBouhali force-pushed the rtl-fix-percentage-direction branch from 17e8f44 to bbd286b Compare October 24, 2022 11:47

arbrandes mentioned this pull request Oct 25, 2022

Reviews for sprint 2022-10-28 openedx/axim-engineering#505

Closed

27 tasks

arbrandes approved these changes Oct 25, 2022

View reviewed changes

arbrandes merged commit fa5cf8f into openedx:master Oct 26, 2022

ARMBouhali deleted the rtl-fix-percentage-direction branch October 27, 2022 06:16

itsjeyd mentioned this pull request Oct 27, 2022

Feature: Persian language #989

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTL: force (%) symbol to follow text direction #978

RTL: force (%) symbol to follow text direction #978

ARMBouhali commented Sep 15, 2022 •

edited

Loading

openedx-webhooks commented Sep 15, 2022 •

edited

Loading

natabene commented Sep 19, 2022

natabene commented Sep 27, 2022

ARMBouhali commented Sep 30, 2022

brian-smith-tcril left a comment

brian-smith-tcril Oct 12, 2022

ARMBouhali Oct 12, 2022 •

edited

Loading

mphilbrick211 commented Oct 12, 2022

natabene commented Oct 13, 2022

codecov bot commented Oct 13, 2022 •

edited

Loading

ARMBouhali commented Oct 13, 2022

brian-smith-tcril left a comment

mphilbrick211 commented Oct 17, 2022

ARMBouhali commented Oct 24, 2022 •

edited

Loading

arbrandes left a comment

ARMBouhali commented Oct 25, 2022

mphilbrick211 commented Oct 26, 2022

openedx-webhooks commented Oct 26, 2022

RTL: force (%) symbol to follow text direction #978

RTL: force (%) symbol to follow text direction #978

Conversation

ARMBouhali commented Sep 15, 2022 • edited Loading

openedx-webhooks commented Sep 15, 2022 • edited Loading

natabene commented Sep 19, 2022

natabene commented Sep 27, 2022

ARMBouhali commented Sep 30, 2022

brian-smith-tcril left a comment

Choose a reason for hiding this comment

brian-smith-tcril Oct 12, 2022

Choose a reason for hiding this comment

ARMBouhali Oct 12, 2022 • edited Loading

Choose a reason for hiding this comment

mphilbrick211 commented Oct 12, 2022

natabene commented Oct 13, 2022

codecov bot commented Oct 13, 2022 • edited Loading

Codecov Report

ARMBouhali commented Oct 13, 2022

brian-smith-tcril left a comment

Choose a reason for hiding this comment

mphilbrick211 commented Oct 17, 2022

ARMBouhali commented Oct 24, 2022 • edited Loading

arbrandes left a comment

Choose a reason for hiding this comment

ARMBouhali commented Oct 25, 2022

mphilbrick211 commented Oct 26, 2022

openedx-webhooks commented Oct 26, 2022

ARMBouhali commented Sep 15, 2022 •

edited

Loading

openedx-webhooks commented Sep 15, 2022 •

edited

Loading

ARMBouhali Oct 12, 2022 •

edited

Loading

codecov bot commented Oct 13, 2022 •

edited

Loading

ARMBouhali commented Oct 24, 2022 •

edited

Loading