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

MWPW-125482 Overriding text flow direction via metatag #777

Merged
merged 16 commits into from
May 26, 2023
Merged

MWPW-125482 Overriding text flow direction via metatag #777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3c4ec90
MWPW-125482 Provoding an option to override default textdirection
aishwaryamathuria May 25, 2023
f569487
MWPW-125482 Provoding an option to override default textdirection
aishwaryamathuria May 25, 2023
05b7963
Update utils.test.js
aishwaryamathuria May 25, 2023
0c185d4
Update scripts.js
aishwaryamathuria May 25, 2023
afabef2
Update utils.test.js
aishwaryamathuria May 25, 2023
0071210
Update scripts.js
aishwaryamathuria May 25, 2023
846c014
Update scripts.js
aishwaryamathuria May 25, 2023
90b4bf2
Adding fallback to intl obj
aishwaryamathuria May 25, 2023
8c58acb
Update utils.test.js
aishwaryamathuria May 25, 2023
df6b1dc
Removing Intl
aishwaryamathuria May 25, 2023
62c62a1
Removing Intl
aishwaryamathuria May 25, 2023
f1b7279
Adding Intl
aishwaryamathuria May 25, 2023
04f292f
Adding Intl
aishwaryamathuria May 25, 2023
7599435
Adding Intl
aishwaryamathuria May 25, 2023
6edbc06
Adding text fix
aishwaryamathuria May 25, 2023
9fc5f72
Adding check for config.locale.ietf to get default ltr
aishwaryamathuria May 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions libs/scripts/scripts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,10 @@ const locales = {
bg: { ietf: 'bg-BG', tk: 'aaz7dvd.css' },
ru: { ietf: 'ru-RU', tk: 'aaz7dvd.css' },
ua: { ietf: 'uk-UA', tk: 'aaz7dvd.css' },
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' },
il_he: { ietf: 'he', tk: 'nwq1mna.css', dir: 'rtl' },
ae_ar: { ietf: 'ar', tk: 'nwq1mna.css', dir: 'rtl' },
mena_ar: { ietf: 'ar', tk: 'dis2dpj.css', dir: 'rtl' },
sa_ar: { ietf: 'ar', tk: 'nwq1mna.css', dir: 'rtl' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Consumers will have to be notified about this change, they need to implement the dir property in their config

Copy link
Contributor Author

@aishwaryamathuria aishwaryamathuria 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.

@narcis-radu Yes.. We will share a notification with the teams on milo-community slack channel before merge.
Please suggest if we need to share the communication on some additional channels.

// Asia Pacific
au: { ietf: 'en-AU', tk: 'pps7abe.css' },
hk_en: { ietf: 'en-HK', tk: 'pps7abe.css' },
Expand Down
3 changes: 2 additions & 1 deletion libs/utils/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ export const [setConfig, getConfig] = (() => {
config.autoBlocks = conf.autoBlocks ? [...AUTO_BLOCKS, ...conf.autoBlocks] : AUTO_BLOCKS;
document.documentElement.setAttribute('lang', config.locale.ietf);
try {
document.documentElement.setAttribute('dir', (new Intl.Locale(config.locale.ietf)).textInfo.direction);
const contentDir = getMetadata('content-direction');
document.documentElement.setAttribute('dir', contentDir || config.locale.dir || (config.locale.ietf ? (new Intl.Locale(config.locale.ietf)?.textInfo?.direction) : null) || 'ltr');
} catch (e) {
// eslint-disable-next-line no-console
console.log('Invalid or missing locale:', e);
Expand Down
6 changes: 3 additions & 3 deletions test/utils/utils.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ describe('Utils', () => {
config.locales = {
'': { ietf: 'en-US', tk: 'hah7vzn.css' },
africa: { ietf: 'en', tk: 'pps7abe.css' },
il_he: { ietf: 'he', tk: 'nwq1mna.css' },
mena_ar: { ietf: 'ar', tk: 'dis2dpj.css' },
il_he: { ietf: 'he', tk: 'nwq1mna.css', dir: 'rtl' },
mena_ar: { ietf: 'ar', tk: 'dis2dpj.css', dir: 'rtl' },
Copy link
Contributor

Choose a reason for hiding this comment

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

make sure your tests cover both ltr and rtl use-cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

make sure your tests cover both ltr and rtl use-cases

@narcis-radu Yes.. 'ltr' tests are already added in the Unit test scripts and are executing successfully.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add tests for rtl as well

Copy link
Contributor

Choose a reason for hiding this comment

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

still need rtl test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrischrischris The rtl and ltr test are already there in the Unit tests.. we have added the dir property for the same in that config .

Copy link
Contributor

@narcis-radu narcis-radu May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

You are correct! But I think one existing test would fail, maybe you can check - https://github.com/adobecom/milo/blob/main/test/utils/utils.test.js#L258

setConfigWithPath('/ua/solutions');
expect(document.documentElement.getAttribute('dir')).null;

Based on what you set - document.documentElement.setAttribute('dir', contentDir || config.locale.dir || new Intl.Locale(config.locale.ietf)?.textInfo?.direction || 'ltr');, this will always return something (ltr default) and we're expecting to be null

Copy link
Contributor Author

@aishwaryamathuria aishwaryamathuria May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@narcis-radu The test was completing successfully because it was only checking that, if the dir attribute setting fails, it should not throw error and the value should be null.
I have added a check to verify that the Intl should run only if config.locale.ietf is set and if not it should not try to find textinfo from it and fallback to 'ltr'
Could you please review?

ua: { tk: 'aaz7dvd.css' },
};
});
Expand All @@ -255,7 +255,7 @@ describe('Utils', () => {

it('Gracefully dies when locale ietf is missing and dir is not set.', () => {
setConfigWithPath('/ua/solutions');
expect(document.documentElement.getAttribute('dir')).null;
expect(document.documentElement.getAttribute('dir')).to.equal('ltr');
});
});

Expand Down