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.

Sign up for GitHub

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

Jump to bottom

MWPW-131454 Use config.locale.dir instead textInfo #773

Closed
wants to merge 2 commits into from
Closed

MWPW-131454 Use config.locale.dir instead textInfo #773

wants to merge 2 commits into from

Conversation

TsayAdobe
Copy link
Contributor

@TsayAdobe TsayAdobe commented May 24, 2023 •
edited by mokimo
Loading

  • Firefox doesn't support textInfo. Add dir in config.locale for rtl languages.
  • If dir is in config.locale, use it. Otherwise, dir is default to ltr.
  • Please use Firefox to test the before and after links.

Resolves: MWPW-131454

Test URLs:

@TsayAdobe TsayAdobe requested a review from a team as a code owner May 24, 2023 21:10
@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented May 24, 2023

Hello, I'm Franklin Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented May 24, 2023

Page Scores Audits Google
/mena_ar/drafts/tsay/mwpw-131454?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP FMP LCP TTI TBT CLS PSI

@codecov
Copy link

codecov bot commented May 24, 2023 •
edited
Loading

Codecov Report

Merging #773 (db231d9) into main (50eaa63) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #773      +/-   ##
==========================================
+ Coverage   95.39%   95.40%   +0.01%     
==========================================
  Files         114      114              
  Lines       29008    29003       -5     
==========================================
  Hits        27671    27671              
+ Misses       1337     1332       -5
Impacted Files Coverage Δ
libs/scripts/scripts.js 100.00% <100.00%> (ø)
libs/utils/utils.js 97.81% <100.00%> (+0.24%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Blainegunn Blainegunn added bug Something isn't working enhancement labels May 24, 2023
@aishwaryamathuria aishwaryamathuria mentioned this pull request May 25, 2023
Merged
@narcis-radu narcis-radu self-requested a review May 25, 2023 07:31
@narcis-radu
Copy link
Contributor

We should add an example in milo-college as well and inform consumers about this change.

mokimo
mokimo reviewed May 25, 2023
View reviewed changes
libs/scripts/scripts.js
Comment on lines -78 to -81
il_he: { ietf: 'he', tk: 'nwq1mna.css' },
ae_ar: { ietf: 'ar', tk: 'nwq1mna.css' },
mena_ar: { ietf: 'ar', tk: 'dis2dpj.css' },
sa_ar: { ietf: 'ar', tk: 'nwq1mna.css' },
Copy link
Contributor

@mokimo mokimo May 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please be aware that every consumer project will need to define this as well, the configs here are purely specific to milo (milo.adobe.com)

@aem-code-sync
Copy link
Contributor

aem-code-sync bot commented May 25, 2023

Page Scores Audits Google
/mena_ar/drafts/tsay/mwpw-131454?martech=off PERFORMANCE A11Y SEO BEST PRACTICES SI FCP FMP LCP TTI TBT CLS PSI

chrischrischris
chrischrischris approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@chrischrischris chrischrischris left a comment

Choose a reason for hiding this comment

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

All consumer locales need to be updated before this can be merged.

@chrischrischris chrischrischris added the do not merge PR should not be merged yet label May 25, 2023
@chrischrischris
Copy link
Contributor

#777 covers this + metadata override, so I believe this one should be closed?

@salonijain3
Copy link
Contributor

yes, @TsayAdobe Please close this PR.

@TsayAdobe
Copy link
Contributor Author

The fix is in #777

@TsayAdobe TsayAdobe closed this May 30, 2023
@TsayAdobe TsayAdobe deleted the MWPW-131454 branch March 18, 2024 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@narcis-radu narcis-radu narcis-radu approved these changes

@chrischrischris chrischrischris chrischrischris approved these changes

@mokimo mokimo mokimo approved these changes

@seanchoi-dev seanchoi-dev seanchoi-dev approved these changes

@Brandon32 Brandon32 Awaiting requested review from Brandon32

@ryanmparrish ryanmparrish Awaiting requested review from ryanmparrish

@rgclayton rgclayton Awaiting requested review from rgclayton

@meganthecoder meganthecoder Awaiting requested review from meganthecoder

Assignees
No one assigned
Labels
bug Something isn't working do not merge PR should not be merged yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@TsayAdobe @narcis-radu @chrischrischris @salonijain3 @mokimo @seanchoi-dev @Blainegunn