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.

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

MWPW-125482 Add dir property to rtl locales #118

Merged
merged 2 commits into from
Jun 7, 2023

Conversation

aishwaryamathuria
Copy link
Contributor

Context

https://github.com/orgs/adobecom/discussions/746

  1. If a page with right-to-left content is previewed on Firefox browser then it gets rendered with left-to-right text flow.
  2. When EN content is used in an RTL locale page, there are instances when the locale of the page is en and content in that locale page needs to be aligned from left to right. Solution was to provide an option override the default rtl alignment.

Why this PR?

adobecom/milo#777
The above mentioned issue and requirement are fixed in milo in the shared PR.

  1. For the alignment issue a new property is added in the content.locales list with each locale named 'dir'. This property is used in place of the current Intl object which was unable to set the dir property in Firefox browser. To make sure the fix works for the project the same property needs to be set in the content.locales for rtl locales so that the issue can be resolved in Firefox browser.

How to use override?

Add the 'content-direction' meta tag with value 'rtl' or 'ltr' in the metadata block of the page or via metadata.xlsx to override the default text alignment.

Resolves: MWPW-125482

Test URLs:

@aem-code-sync
Copy link

aem-code-sync bot commented Jun 1, 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

aem-code-sync bot commented Jun 1, 2023

Page Scores Audits Google
/mena_ar/customer-success-stories/dixons-carphone-case-study?martech=off Lighthouse returned error: ERRORED_DOCUMENT_REQUEST. Lighthouse was unable to reliably load the page you requested. Make sure you are testing the correct URL and that the server is properly responding to all requests. (Status code: 401) PSI

@codecov-commenter
Copy link

Codecov Report

Merging #118 (a9fa8ad) into main (7afc073) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #118   +/-   ##
=======================================
  Coverage   96.36%   96.36%           
=======================================
  Files           6        6           
  Lines         468      468           
=======================================
  Hits          451      451           
  Misses         17       17           

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

@aishwaryamathuria
Copy link
Contributor Author

@meganthecoder @Brandon32 @JasonHowellSlavin
I do not have permission to merge the PR.. Could you merge in case there are no additional approvals required?

@meganthecoder
Copy link
Contributor

@aishwaryamathuria This needs to be verified first, which I believe @Dli3 is currently working on. Please review our contributing doc for further details on our process: https://github.com/adobecom/bacom/blob/main/CONTRIBUTING.md

@Dli3
Copy link
Contributor

Dli3 commented Jun 5, 2023

Hi @aishwaryamathuria , we're our PM to review the bugs reported in the MWPW-125482 ticket.

@Dli3 Dli3 added the verified it's been E2E tested by someone (that isn't the PR requestor) label Jun 7, 2023
@Dli3
Copy link
Contributor

Dli3 commented Jun 7, 2023

Verified, thanks @aishwaryamathuria .
cc @meganthecoder @JasonHowellSlavin @Brandon32

@Dli3 Dli3 merged commit 0bf32bf into adobecom:main Jun 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
verified it's been E2E tested by someone (that isn't the PR requestor)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants