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

Jump to bottom

RangeControl: Fix space between icons and rail #36935

Merged
merged 3 commits into from
Jan 24, 2022
Merged

RangeControl: Fix space between icons and rail #36935

merged 3 commits into from
Jan 24, 2022

Conversation

imangm
Copy link
Contributor

@imangm imangm commented Nov 28, 2021 •
edited by gziolo
Loading

Description

Fixes #37801.

Space between beforeIcon and range control and between afterIcon and range control were not equal.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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 (please manually search all *.native.js files for terms that need renaming or removal).

@imangm
style fix - space between icons and rail
d2a36c6 
Space between beforeIcon and range control and between afterIcon and range control were not equal.
@imangm imangm requested a review from ajitbohra as a code owner November 28, 2021 16:29
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @imangm! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Nov 28, 2021
@Mamaduka
Copy link
Member

Mamaduka commented Jan 6, 2022

Hello, @imangm

Sorry for the late follow-up. Is there an issue we should link to this PR?

@Mamaduka Mamaduka added the [Package] Components /packages/components label Jan 6, 2022
@Mamaduka Mamaduka changed the title style fix - space between icons and rail RangeControl: Fix space between icons and rail Jan 6, 2022
@imangm imangm mentioned this pull request Jan 8, 2022
Closed
@imangm
Copy link
Contributor Author

imangm commented Jan 8, 2022

Hello, @imangm

Sorry for the late follow-up. Is there an issue we should link to this PR?

Hi @Mamaduka,

Actually, I hadn't done it since I didn't know about the process. But I did it now: #37801

Thanks

@ciampo ciampo requested review from ciampo and mirka January 10, 2022 08:15
@gziolo gziolo added the [Type] Bug An existing feature does not function as intended label Jan 10, 2022
@ciampo ciampo requested a review from jasmussen January 11, 2022 10:18
ciampo
ciampo reviewed Jan 11, 2022
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @imangm , thank you for opening this PR! I'm not aware if the discrepancy in the spacing between the before and the after icons is there on purpose or not. I wonder if @jasmussen can help out on this (issue reported in #37801)

Also, would you mind adding an entry to the CHANGELOG?

jasmussen
jasmussen reviewed Jan 11, 2022
View reviewed changes
packages/components/src/range-control/styles/range-control-styles.js
@@ -58,7 +58,7 @@ export const BeforeIconWrapper = styled.span`
export const AfterIconWrapper = styled.span`
margin-top: ${ railHeight }px;

${ rtl( { marginLeft: 16 } ) }
${ rtl( { marginLeft: 6 } ) }
Copy link
Contributor

Choose a reason for hiding this comment

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

This change seems fine since it matches the metrics of the BeforeIconWrapper 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.

Cool, I just wasn't sure if the difference was on purpose or not.

Copy link
Contributor

@ciampo ciampo Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you!

@jasmussen
Copy link
Contributor

Thanks for the PR, it looks pretty safe to me. To the best of my knowledge, the afterIcon property hasn't been widely used in the block editor itself, and exists only in storybook and tests, which is likely why it might not have received as much attention. Same, honestly, with the beforeIcon, so it seems like a good enough patch to make!

Unrelated to this PR, it might be nice to look at using gap on the component side, so we could do with a single value rather than two.

ciampo
ciampo approved these changes Jan 11, 2022
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

This can be merged as soon as a changelog entry gets added

@ciampo
Copy link
Contributor

ciampo commented Jan 20, 2022

LGTM 🚀

This can be merged as soon as a changelog entry gets added

Hey @imangm, do you need any help? This is the CHANGELOG I was referring to.

@imangm
Copy link
Contributor Author

imangm commented Jan 24, 2022

LGTM 🚀
This can be merged as soon as a changelog entry gets added

Hey @imangm, do you need any help? This is the CHANGELOG I was referring to.

Hey @ciampo! Not sure if I've done it properly since it's my first time doing it here, but I fetched the latest upstream version and updated changelog and committed it. Is it fine?

@ciampo
Copy link
Contributor

ciampo commented Jan 24, 2022

LGTM 🚀
This can be merged as soon as a changelog entry gets added

Hey @imangm, do you need any help? This is the CHANGELOG I was referring to.

Hey @ciampo! Not sure if I've done it properly since it's my first time doing it here, but I fetched the latest upstream version and updated changelog and committed it. Is it fine?

Looks good to me, thank you! Going to merge now

@ciampo ciampo merged commit afa6767 into WordPress:trunk Jan 24, 2022
@github-actions github-actions bot added this to the Gutenberg 12.5 milestone Jan 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasmussen jasmussen jasmussen left review comments

@ciampo ciampo ciampo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@mirka mirka Awaiting requested review from mirka

Assignees

@imangm imangm

Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 12.5
Development

Successfully merging this pull request may close these issues.

RangeControl: Spacing issue between icons and the rail
5 participants
@imangm @Mamaduka @jasmussen @ciampo @gziolo