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

Fixes #34855 RangeControl style issue in RTL #35777

Merged
merged 3 commits into from
Oct 25, 2021

Conversation

pbearne
Copy link
Contributor

@pbearne pbearne commented Oct 19, 2021

Description

Fixes #34855 RangeControl style issue in RTL

How has this been tested?

Switch RTL lag (hebrew)
load the space block
check the

Screenshots

Types of changes

Adjusted style for RTL

Checklist:

  • [x ] My code is tested.
  • [x ] My code follows the WordPress code style.
  • [x ] My code follows the accessibility standards.
  • [x ] I've tested my changes with keyboard and screen readers.
  • [x ] My code has proper inline documentation.
  • [x ] I've included developer documentation if appropriate.
  • [x ] I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@ntsekouras ntsekouras requested a review from jasmussen October 20, 2021 12:10
@ntsekouras ntsekouras added the [Package] Components /packages/components label Oct 20, 2021
@jasmussen jasmussen requested a review from ciampo October 20, 2021 12:14
@@ -192,7 +192,7 @@ export const ThumbWrapper = styled.span`
transform: translateX( 4.5px );

${ thumbColor };
${ rtl( { marginLeft: -10 } ) };
${ rtl( { marginLeft: -10 }, { marginRight: 1 } ) };
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it makes sense that a symmetrical element like this requires asymmetrical margins. In fact the translateX( 4.5px ) a few lines above stands out to me as something that is not being RTL'ed yet. That might also fix the tooltip, which still isn't perfectly aligned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change the translate didn't fix it
but I was able to tweak the position of the dot so it lines up better
image
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, have we tried something like this yet? The reason I would prefer a symmetric solution if possible is that it's easier to fix when an underlying size value changes. Less guess work and manual tweaking involved.

-	transform: translateX( 4.5px );

	${ thumbColor };
-	${ rtl( { marginLeft: -10 }, { marginRight: 1 } ) };
+	${ rtl( { marginLeft: -10 } ) };
+	${ rtl(
+		{ transform: 'translateX( 4.5px )' },
+		{ transform: 'translateX( -4.5px )' }
+	) };

Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for fixing!

@mirka mirka merged commit 2bb4e77 into WordPress:trunk Oct 25, 2021
@github-actions github-actions bot added this to the Gutenberg 11.9 milestone Oct 25, 2021
@vcanales vcanales added the [Type] Bug An existing feature does not function as intended label Oct 27, 2021
vcanales pushed a commit that referenced this pull request Oct 27, 2021
* Fixes #34855 RangeControl style issue in RTL

* addjest the dot offset so it lines up with tool tip

* swapped to using translateX for the adjustment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RangeControl style issue in RTL
4 participants