Skip to content

Button: Properly handle border radius reset#23887

Merged
ItsJonQ merged 1 commit into
WordPress:masterfrom
njbrown:bug/button-border-radius-reset-fix
Jul 13, 2020
Merged

Button: Properly handle border radius reset#23887
ItsJonQ merged 1 commit into
WordPress:masterfrom
njbrown:bug/button-border-radius-reset-fix

Conversation

@njbrown

@njbrown njbrown commented Jul 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

youknowriad commented Jul 13, 2020

Copy link
Copy Markdown
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

ItsJonQ commented Jul 13, 2020 •
edited
Loading

Copy link
Copy Markdown

@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

youknowriad commented Jul 13, 2020

Copy link
Copy Markdown
Contributor

Yes, that sounds good to me.

@ItsJonQ ItsJonQ self-requested a review July 13, 2020 15:44
ItsJonQ
ItsJonQ approved these changes Jul 13, 2020
View reviewed changes

@ItsJonQ ItsJonQ left a comment

Copy link
Copy Markdown

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

github-actions Bot commented Jul 13, 2020

Copy link
Copy Markdown

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
youknowriad
youknowriad reviewed Jul 13, 2020
View reviewed changes
Comment thread packages/block-library/src/button/edit.js
setAttributes( {
borderRadius: initialBorderRadius,
} );
else setAttributes( { borderRadius: newBorderRadius } );

@youknowriad youknowriad Jul 13, 2020

Copy link
Copy Markdown
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.

@ItsJonQ ItsJonQ Jul 13, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh, hmm 🤔

@youknowriad youknowriad Jul 13, 2020

Copy link
Copy Markdown
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.

youknowriad pushed a commit that referenced this pull request Jul 13, 2020
@njbrown @youknowriad
Update button when borderRadius resets (#23887)
6eb9e2b
@youknowriad youknowriad mentioned this pull request Jul 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowriad youknowriad youknowriad left review comments
@ItsJonQ ItsJonQ ItsJonQ approved these changes
@ajitbohra ajitbohra Awaiting requested review from ajitbohra
@Soean Soean Awaiting requested review from Soean
@talldan talldan Awaiting requested review from talldan

Assignees

No one assigned

Labels

[Block] Buttons Affects the Buttons Block First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository

Projects

None yet

Milestone

Gutenberg 8.6

Development

Successfully merging this pull request may close these issues.

3 participants

@njbrown @youknowriad @ItsJonQ