Improves RTL style conversion #20503
Conversation
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten,
jorgefilipecosta and
youknowriad
as code owners
|
Size Change: +64 B (0%) Total Size: 866 kB
ℹ️ View Unchanged
|
|
@aduth Ah yes! Thank you so much <3. I really appreciate your thoroughness and feedback! I guess I got a little tunnel-visioned on just the left -> right aspect of it. I just made an update and added tests for left -> right, and right -> left. |
|
Now I can add the edge case test I had planned to add yesterday 😄 See. 474455450 (currently failing) Related: https://developer.mozilla.org/en-US/docs/Web/CSS/text-combine-upright |
|
@aduth TIL about |
|
@ItsJonQ I had never heard of it previously either 😄 Just pulled it as an example looking through a full list of CSS properties for substrings of "left" and "right" which we'd not expect to flip in the sense of horizontal directionality. |
packages/components/src/utils/rtl.js
Outdated
| if ( lowerLeftRegExp.test( key ) ) { | ||
| nextKey = key.replace( lowerLeftRegExp, '-right' ); | ||
| } |
There was a problem hiding this comment.
Per my reference comment, would there be any difference if this was implemented as:
| if ( lowerLeftRegExp.test( key ) ) { | |
| nextKey = key.replace( lowerLeftRegExp, '-right' ); | |
| } | |
| nextKey = key.replace( lowerLeftRegExp, '-right' ); |
There was a problem hiding this comment.
@aduth I just tried. It breaks one of the conversions in the unit tests
convertLtrToRtl
✓ converts (*)Left <-> (*)Right (6ms)
✕ converts (*)left <-> (*)right (7ms)
● convertLtrToRtl › converts (*)left <-> (*)right
expect(received).toBe(expected) // Object.is equality
Expected: 10
Received: 20
105 | expect( nextStyle[ 'scroll-margin-right' ] ).toBe( 10 );
106 | expect( nextStyle[ 'scroll-padding-right' ] ).toBe( 10 );
> 107 | expect( nextStyle.right ).toBe( 10 );
| ^
108 |
109 | // right -> left
110 | expect( nextStyle[ 'border-left' ] ).toBe( '20px solid blue' );
at Object.toBe (packages/components/src/utils/test/rtl.js:107:29)
There was a problem hiding this comment.
I think the reason is that we should be doing the replace as operating on the working copy of nextKey.
| if ( lowerLeftRegExp.test( key ) ) { | |
| nextKey = key.replace( lowerLeftRegExp, '-right' ); | |
| } | |
| nextKey = nextKey.replace( lowerLeftRegExp, '-right' ); |
packages/components/src/utils/rtl.js
Outdated
| const lowerLeftRegExp = new RegExp( /-left/g ); | ||
| const lowerRightRegExp = new RegExp( /-right/g ); | ||
| const upperLeftRegExp = new RegExp( /Left/g ); | ||
| const upperRightRegExp = new RegExp( /Right/g ); |
There was a problem hiding this comment.
They look like constants to me.
https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/#constants
| const lowerLeftRegExp = new RegExp( /-left/g ); | |
| const lowerRightRegExp = new RegExp( /-right/g ); | |
| const upperLeftRegExp = new RegExp( /Left/g ); | |
| const upperRightRegExp = new RegExp( /Right/g ); | |
| const LOWER_LEFT_REGEXP = new RegExp( /-left/g ); | |
| const LOWER_RIGHT_REGEXP = new RegExp( /-right/g ); | |
| const UPPER_LEFT_REGEXP = new RegExp( /Left/g ); | |
| const UPPER_RIGHT_REGEXP = new RegExp( /Right/g ); |
packages/components/src/utils/rtl.js
Outdated
| if ( lowerLeftRegExp.test( key ) ) { | ||
| nextKey = key.replace( lowerLeftRegExp, '-right' ); | ||
| } |
There was a problem hiding this comment.
I think the reason is that we should be doing the replace as operating on the working copy of nextKey.
| if ( lowerLeftRegExp.test( key ) ) { | |
| nextKey = key.replace( lowerLeftRegExp, '-right' ); | |
| } | |
| nextKey = nextKey.replace( lowerLeftRegExp, '-right' ); |
|
More a minor point, but could become a scaling issue: We should be able to stop immediately once we reach a match of any of these conditions, rather than continue to test them. It would probably be a refactor to a separate function using function getConvertedKey( key ) {
if ( key === 'left' ) {
return 'right';
}
if ( key === 'right' ) {
return 'left';
}
return key
.replace( LOWER_LEFT_REGEXP, '-right' )
.replace( LOWER_RIGHT_REGEXP, '-left' )
.replace( UPPER_LEFT_REGEXP, 'Right' )
.replace( UPPER_RIGHT_REGEXP, 'Left' );
}Though now that I write this out, it only really benefits the direct |
|
Actually, in testing that, I see now the problem with always running So a correct optimized version would be: function getConvertedKey( key ) {
if ( key === 'left' ) {
return 'right';
}
if ( key === 'right' ) {
return 'left';
}
if ( LOWER_LEFT_REGEXP.test( key ) ) {
return key.replace( LOWER_LEFT_REGEXP, '-right' );
}
if ( LOWER_RIGHT_REGEXP.test( key ) ) {
return key.replace( LOWER_RIGHT_REGEXP, '-left' );
}
if ( UPPER_LEFT_REGEXP.test( key ) ) {
return key.replace( UPPER_LEFT_REGEXP, 'Right' );
}
if ( UPPER_RIGHT_REGEXP.test( key ) ) {
return key.replace( UPPER_RIGHT_REGEXP, 'Left' );
}
return key;
} |
@aduth That's what I ran into as well 😂 . The chained replace would work if there were unique temporary tokens injected, which would be converted to desired values (at the very end). Your refactor suggestion is great! I'll make the update |
|
Another possible simplification: diff --git a/packages/components/src/utils/rtl.js b/packages/components/src/utils/rtl.js
index 317d8eb4a..cf366412e 100644
--- a/packages/components/src/utils/rtl.js
+++ b/packages/components/src/utils/rtl.js
@@ -2,6 +2,7 @@
* External dependencies
*/
import { css } from '@emotion/core';
+import { mapKeys } from 'lodash';
const LOWER_LEFT_REGEXP = new RegExp( /-left/g );
const LOWER_RIGHT_REGEXP = new RegExp( /-right/g );
@@ -68,17 +69,8 @@ function getConvertedKey( key ) {
*
* @return {Object} Converted ltr -> rtl styles
*/
-export const convertLtrToRtl = ( ltrStyles = {} ) => {
- const nextStyles = {};
-
- for ( const key in ltrStyles ) {
- const value = ltrStyles[ key ];
- const nextKey = getConvertedKey( key );
- nextStyles[ nextKey ] = value;
- }
-
- return nextStyles;
-};
+export const convertLtrToRtl = ( ltrStyles ) =>
+ mapKeys( ltrStyles, ( _value, key ) => getConvertedKey( key ) );
/**
* A higher-order function that create an incredibly basic ltr -> rtl style converter for CSS objects. |
packages/components/src/utils/rtl.js
Outdated
| const convertLtrToRtl = ( ltrStyles = {} ) => { | ||
| export const convertLtrToRtl = ( ltrStyles = {} ) => { |
There was a problem hiding this comment.
On the topic of case standards, there's also:
i.e. This should technically be convertLTRToRTL
There was a problem hiding this comment.
Thank you!! Interesting question... In that case, should the main function be renamed from rtl() to RTL()?
There was a problem hiding this comment.
rtl would be the correct capitalization. The phrasing is a bit strange, but it's meant to be covered by this bit of the standard:
If an abbreviation or an acronym occurs at the start of a variable name, it must be written to respect the camelcase naming rules covering the first letter of a variable or class definition. For variable assignment, this means writing the abbreviation entirely as lowercase.
|
Hmm.. not sure how to re-run Travis. I clicked "Re-run" on this page, but it didn't seem to do anything |
This update improves the rtl style utility to better handle varying combinations of `left` and `Left` based properties. A handful of tests have been added to ensure style property conversions are outputted as expected.
Also adds tests
aduth
force-pushed
the
fix/css-js-rtl
branch
from
c8bf91c to
1b95d4f
Compare
Description
This update improves the rtl style utility to better handle varying combinations of
leftandLeftbased properties. A handful of tests have been added to ensure style property conversions are outputted as expected.Types of changes
leftandLeftreplacements in thertlconvertorChecklist:
Addresses (🤞) @aduth 's comments from https://github.com/WordPress/gutenberg/pull/19916/files/110831f8f61f5e43ff9c76331159415040b412c9#r379631860