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

i18n: Add isRTL function #20298

Merged
merged 4 commits into from
Feb 26, 2020
Merged

i18n: Add isRTL function #20298

merged 4 commits into from
Feb 26, 2020

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Feb 18, 2020

Description

Add isRTL function for checking whether the locale is right-to-left.

This follows the implementation of WP_Locale::is_rtl().

https://developer.wordpress.org/reference/classes/wp_locale/is_rtl/

Part of #19212.

The unit tests have been slightly restructured to provide isolation of the i18n module between tests.

How has this been tested?

Includes unit tests.

Types of changes

New feature (non-breaking change which adds functionality)

Add isRTL function to @wordpress/i18n.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

- Better test isolation by re-requiring the module
- Add `beforeEach` setting up default locale data
- Add isRTL tests
@sirreal sirreal added [Status] In Progress Tracking issues with work in progress Internationalization (i18n) Issues or PRs related to internationalization efforts [Package] i18n /packages/i18n labels Feb 18, 2020
@sirreal sirreal self-assigned this Feb 18, 2020
@sirreal sirreal added Needs Technical Feedback Needs testing from a developer perspective. and removed [Status] In Progress Tracking issues with work in progress labels Feb 18, 2020
@sirreal sirreal marked this pull request as ready for review February 18, 2020 21:19
@sirreal sirreal requested a review from swissspidy as a code owner February 18, 2020 21:19
@sirreal sirreal requested review from aduth and jsnajdr February 18, 2020 21:21
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Code looks good, thanks for adding docs and improving tests!

I haven't tested this in any way other than confirming that tests run.

@youknowriad
Copy link
Contributor

Interesting way of checking the rtl direction :)
We have a few different ways of doing it already in the code base, do you think we should update our existing checks to use this new function?

@sirreal
Copy link
Member Author

sirreal commented Feb 19, 2020

Interesting way of checking the rtl direction :)

It seems to be the canonical way of specifying direction in WordPress. See https://github.com/WordPress/WordPress/blob/511a297fb4b89ad32e89ea0373347cf92367e1c3/wp-includes/class-wp-locale.php#L217-L219

We have a few different ways of doing it already in the code base, do you think we should update our existing checks to use this new function?

That's what #19212 suggests. I agree that there's value in having a single, well-defined way of checking the text direction.

@simison
Copy link
Member

simison commented Feb 19, 2020

We have a few different ways of doing it already in the code base, do you think we should update our existing checks to use this new function?

Spotted here:

function getRtl() {
return !! ( document && document.documentElement.dir === 'rtl' );
}

@youknowriad
Copy link
Contributor

yes, we also have a setting in the block-editor store I believe and another check in the Popover component.

@simison
Copy link
Member

simison commented Feb 20, 2020

@youknowriad would you expect to see those uses updated in this PR or good for follow-ups?

@youknowriad
Copy link
Contributor

I'm totally fine if we merge this as is :)

But I've been promised follow-ups in the past and sometimes they just don't happen :P

@sirreal
Copy link
Member Author

sirreal commented Feb 24, 2020

I'll merge this in the next few days unless there are issues or there's a good reason to wait.

I didn't plan to modify anything beyond @wordpress/i18n with this, but I can certainly help to migrate existing functionality to use isRTL in follow-up changes.

@sirreal sirreal merged commit 2ff5493 into master Feb 26, 2020
@sirreal sirreal deleted the add/i18n-rtl branch February 26, 2020 12:27
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Feb 26, 2020
@aduth
Copy link
Member

aduth commented Feb 26, 2020

Arriving to this late since I'm only recently returning from AFK, but seems like a reasonable addition to me 👍

@sirreal
Copy link
Member Author

sirreal commented Feb 26, 2020

We have a few different ways of doing it already in the code base, do you think we should update our existing checks to use this new function?

I started to look at this and have some concerns. There's currently a mix of reactive (re-renders as necessary) and non-reactive RTL lookups.

For example, this "hook" looks reactive, but isn't:

function getRtl() {
return !! ( document && document.documentElement.dir === 'rtl' );
}
/**
* Simple hook to retrieve RTL direction value
*/
export function useRtl() {
return getRtl();
}

This is reactive, but doesn't know anything about RTL and is reading a value from a data store:

const isRTL = useSelect( ( select ) => {
return !! select( 'core/block-editor' ).getSettings().isRTL;
}, [] );

Then there are straightforward cases like this:

const isRTL = document.documentElement.dir === 'rtl';


Replacing the reactive RTL setting lookups with isRTL at this time seems undesirable. Would the isRTL setting disappear from block-editor?

$settings = array(
'disableCustomColors' => get_theme_support( 'disable-custom-colors' ),
'disableCustomFontSizes' => get_theme_support( 'disable-custom-font-sizes' ),
'imageSizes' => $available_image_sizes,
'isRTL' => is_rtl(),
'maxUploadFileSize' => $max_upload_size,
);

I'd like to see RTL information come from the locale and be updated in a reactive way. Perhaps is worth waiting for #19210 to be resolved?

@youknowriad
Copy link
Contributor

Replacing the reactive RTL setting lookups with isRTL at this time seems undesirable. Would the isRTL setting disappear from block-editor?

I think it's fine, we just need to add a dev note about it. It's unlikely that anyone aside us is using that setting right now.

@sirreal
Copy link
Member Author

sirreal commented Jun 1, 2020

I've started some work on using isRTL throughout Gutenberg: #22806

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internationalization (i18n) Issues or PRs related to internationalization efforts Needs Technical Feedback Needs testing from a developer perspective. [Package] i18n /packages/i18n
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants