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

Button: Properly handle border radius reset #23887

Merged
merged 1 commit into from
Jul 13, 2020
Merged

Button: Properly handle border radius reset #23887

merged 1 commit into from
Jul 13, 2020

Conversation

njbrown
Copy link
Contributor

@njbrown njbrown commented Jul 12, 2020

Description

When the reset button of the border radius slider is clicked, the button in the editor doesn't reflect the change. This bug exists because the RangeControl component passed undefined to its onChange callback when the reset button is hit and this condition was not handled.

This pr fixes it by capturing the initial value of the border radius of the button then resets to it to that value when the reset button is pressed.

How has this been tested?

Add a Button
Change the value of the border radius
Hit the reset button beside the border radius slider

Screenshots

before fix:
roudedbordersbroken

after fix:
roudedbordersfixed

Types of changes

Bug Fix

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.

@youknowriad
Copy link
Contributor

Thanks for the PR @njbrown

Feels like this issue has been fixed several times already without success :P
Do you know what's the current state here @ItsJonQ and is this a good approach?

@ItsJonQ ItsJonQ added the [Block] Buttons Affects the Buttons Block label Jul 13, 2020
@ItsJonQ
Copy link

ItsJonQ commented Jul 13, 2020

@njbrown Thank you for working on this! ❤️

Do you know what's the current state here @ItsJonQ and is this a good approach?

@youknowriad I took a look. There's something odd about the reset mechanism of this component. I remember helping fix something RangeControl related where the expected result (on reset) was null rather than initialPosition.

There's also a resetFallbackValue prop on the component, which appears to be designed to handle this, but it doesn't look like it's used in the code base.

Ideally, the consumer of this component shouldn't have to manually interpret change callbacks to manage resets.

Digging through the closed issues.. the previous bug came from 0 handling, rather than reset:
#22393


Looking at the previous implementation 🤔

https://github.com/WordPress/gutenberg/blob/rnmobile/release-1.12.0/packages/components/src/range-control/index.js#L39

It looks like the reset action caused onChange() to be called without any arguments. Technically, the functionality would have been the same before compared to now.


As a next step, I'll observe and track issues related to RangeControl reset handling to see if I notice continued divergence in how it's implemented.

For now, I think this update works.

Does that sound good?

Thanks!

@youknowriad
Copy link
Contributor

Yes, that sounds good to me.

@ItsJonQ ItsJonQ self-requested a review July 13, 2020 15:44
Copy link

@ItsJonQ ItsJonQ 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 to me! Thank you @njbrown !!

I left a note regarding better reset handling overall. It's not a blocker for this PR.

Approved!

@ItsJonQ ItsJonQ merged commit bdfdd73 into WordPress:master Jul 13, 2020
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jul 13, 2020
@github-actions
Copy link

Congratulations on your first merged pull request, @njbrown! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 8.6 milestone Jul 13, 2020
setAttributes( {
borderRadius: initialBorderRadius,
} );
else setAttributes( { borderRadius: newBorderRadius } );
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird that this passes the linting checks, I thought {} were enforced.

Copy link

Choose a reason for hiding this comment

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

Oh, hmm 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big deal, but if folks think it could be a good rule we can see if we should add it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants