Fixed Range Control Reset Button #14919

rebeccaj1211 · 2019-04-11T01:10:42Z

Description

Originally, the range control bar would move to original position when the reset button was pressed but the number stayed the same.

Now, when you press the reset button the position moves to the original position (if specified) and default number (added new property). For the current blocks that utilize this, they do not have this new property so it will default to -1 whatever default is set in attributes.

~~The default value for RSS is 5, so I manually added that property for testing purposes but also so it's one less block that needs to be modified to use the default value.~~ The default value for RSS is 5 (which was passed in through its attributes). However, this attribute would change each time the block was rendered so we had to save it in state.

See #14166

How has this been tested?

I manually tested it and it passes all unit test cases.

Screenshots

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

packages/components/src/range-control/index.js

packages/block-library/src/rss/edit.js

packages/components/src/range-control/index.js

gziolo · 2019-04-26T07:34:37Z

packages/block-library/src/rss/edit.js

@@ -119,6 +120,8 @@ class RSSEdit extends Component {
 							onChange={ ( value ) => setAttributes( { itemsToShow: value } ) }
 							min={ DEFAULT_MIN_ITEMS }
 							max={ DEFAULT_MAX_ITEMS }
+							allowReset={ true }
+							initialValue={ this.state.initialItemsToShow }


I noticed that there is already initialPosition prop which plays exactly the same role. We should use it instead. See:
https://github.com/WordPress/gutenberg/tree/c2c112846126e786939c2525a2c802d6016a2c82/packages/components/src/range-control#initialposition

The initialPosistion prop updates each time the slider button is changed. If I simply reset to initialPosition, nothing would change. This is why I created the initialValue

The initialPosistion prop updates each time the slider button is changed. If I simply reset to initialPosition, nothing would change. This is why I created the initialValue

@jorgefilipecosta - I see that you have added initialPosition in #6088. Can you clarify its intended behavior?

The initialPosition property allows the slider circle to appear as if a given value was select, when in fact the value is undefined.

This was the functionality that allowed the font size picker to appear in a position that makes sense instead of appearing in the middle when the font size is unset.
The font size picker was meanwhile redesigned and core is not using this feature anymore, but I guess we need to keep it in order to be back-compatible with existing plugins.

Interesting, this means it's something different and its description should be updated to better reflect it. I remember what was the case, we wanted to show the current font size inherited from the CSS used by the theme author.

gziolo · 2019-04-26T07:38:12Z

packages/components/src/range-control/index.js

 	...props
 } ) {
 	const id = `inspector-range-control-${ instanceId }`;
 	const currentInputValue = currentInput === null ? value : currentInput;
+	const initialInputValue = ! ( initialValue && min <= initialValue && max >= initialValue ) ?


It would read easier if the check would start with a positive condition.

Suggested change

const initialInputValue = ! ( initialValue && min <= initialValue && max >= initialValue ) ?

const initialInputValue = ( initialPosition && initialPosition >= min && initialPosition <= max ) ? parseFloat( initialPosition ) : undefined;

So I used initialValue instead of initialPosition for reason above.

gziolo · 2019-04-26T07:40:21Z

It tests well. I left 2 additional comments where at least the one about reusing the existing initialPosition prop has to be addressed. I also flagged this PR for accessibility review.

afercia · 2019-05-09T08:05:00Z

Originally, the range control bar would move to original position when the reset button was pressed but the number stayed the same.

Right, seems to me this is a bug 🙂I'd be all for fixing it, thanks for your PR!

Worth noting there are other usability and accessibility concerns regarding the Range Control so I've edited the "Fixes" in this PR original description and changed it to "See". In fact, this PR is a welcomed improvements but can't close #14166.

Other considerations

1
I've noticed the "Reset" button gets disabled with a disabled HTML attribute. This is a pre-existing issue but worth reminding again we can't simply disable a UI control while it has keyboard focus, because that will produce a focus loss. For other similar cases we're no-oping the control and using aria-disabled instead. Will open a separate issue.

2
Seems to me in the RSS component there are other range controls with a default value (and maybe other ones in other components?). For example, when enabling "Display Excerpt":

or when switching to the Grid layout:

Should these range controls have a reset button as well?

3
Styling: the "Reset" button needs some styling. See in the screenshot below. On the left: normal state, on the right: focused:

Compared with the reset button in the font size picker:

Seems to me this button needs the following props to start with:

className="components-range-control__clear"
isSmall
isDefault

then it will need some spacing on the left.

Will leave other considerations to @gziolo 🙂

gziolo · 2019-05-16T16:09:46Z

Styling: the "Reset" button needs some styling. See in the screenshot below. On the left: normal state, on the right: focused:

Compared with the reset button in the font size picker:

Seems to me this button needs the following props to start with:
className="components-range-control__clear"
isSmall
isDefault

@mapk - can we get some design help here?

mapk · 2019-05-18T02:58:19Z

can we get some design help here?

Thanks for the ping! Definitely. Let's go with a Reset button like the two examples below. This seems to be the emerging pattern for Gutenberg right now.

Columns reset

Font reset

Mamaduka · 2021-09-07T12:22:32Z

Thanks so much for working on this PR. It looks like the RangeControls component got fully revamped, and that addressed the same issue.

I'm going to close this out as a result.

fixed issue WordPress#14166

6c94cd7

rebeccaj1211 requested review from aduth, ajitbohra, chrisvanpatten, gziolo, jorgefilipecosta, noisysocks, notnownikki, Soean, talldan and youknowriad as code owners April 11, 2019 01:10

rebeccaj1211 changed the title ~~fixed issue #14166~~ Fixed Range Control Reset Button Apr 11, 2019

Soean added the [Block] RSS Affects the RSS Block - used to display entries from an RSS/Atom feed label Apr 11, 2019

gziolo reviewed Apr 11, 2019

View reviewed changes

packages/components/src/range-control/index.js Outdated Show resolved Hide resolved

gziolo added [Feature] UI Components Impacts or related to the UI component system [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). labels Apr 11, 2019

Rebecca Jiang added 2 commits April 11, 2019 18:09

Merge remote-tracking branch 'upstream/master'

fa43b33

added fixes

ed06e45

gziolo requested changes Apr 12, 2019

View reviewed changes

packages/components/src/range-control/index.js Outdated Show resolved Hide resolved

packages/block-library/src/rss/edit.js Outdated Show resolved Hide resolved

packages/components/src/range-control/index.js Outdated Show resolved Hide resolved

added fixes

c2c1128

gziolo added Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code Needs Accessibility Feedback Need input from accessibility labels Apr 19, 2019

gziolo reviewed Apr 26, 2019

View reviewed changes

added suggested fix for readability

8d84189

afercia removed the Needs Accessibility Feedback Need input from accessibility label May 9, 2019

afercia mentioned this pull request May 9, 2019

Don't use disabled on focusable UI controls e.g. "Reset" buttons #15524

Open

gziolo assigned rebeccaj1211 May 10, 2019

gziolo added the Needs Design Feedback Needs general design feedback. label May 16, 2019

gziolo removed the Needs Design Feedback Needs general design feedback. label May 21, 2019

gziolo added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Jun 3, 2019

Base automatically changed from master to trunk March 1, 2021 15:42

Mamaduka closed this Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed Range Control Reset Button #14919

Fixed Range Control Reset Button #14919

rebeccaj1211 commented Apr 11, 2019 •

edited by afercia

Loading

gziolo Apr 26, 2019

rebeccaj1211 Apr 30, 2019

gziolo May 16, 2019

jorgefilipecosta May 20, 2019

gziolo May 21, 2019

gziolo Apr 26, 2019

rebeccaj1211 Apr 30, 2019

gziolo commented Apr 26, 2019

afercia commented May 9, 2019

gziolo commented May 16, 2019

mapk commented May 18, 2019

Mamaduka commented Sep 7, 2021

	const initialInputValue = ! ( initialValue && min <= initialValue && max >= initialValue ) ?
	const initialInputValue = ( initialPosition && initialPosition >= min && initialPosition <= max ) ? parseFloat( initialPosition ) : undefined;

Fixed Range Control Reset Button #14919

Fixed Range Control Reset Button #14919

Conversation

rebeccaj1211 commented Apr 11, 2019 • edited by afercia Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

gziolo Apr 26, 2019

Choose a reason for hiding this comment

rebeccaj1211 Apr 30, 2019

Choose a reason for hiding this comment

gziolo May 16, 2019

Choose a reason for hiding this comment

jorgefilipecosta May 20, 2019

Choose a reason for hiding this comment

gziolo May 21, 2019

Choose a reason for hiding this comment

gziolo Apr 26, 2019

Choose a reason for hiding this comment

rebeccaj1211 Apr 30, 2019

Choose a reason for hiding this comment

gziolo commented Apr 26, 2019

afercia commented May 9, 2019

Other considerations

gziolo commented May 16, 2019

mapk commented May 18, 2019

Mamaduka commented Sep 7, 2021

rebeccaj1211 commented Apr 11, 2019 •

edited by afercia

Loading