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

RangeControl - tick marks, withInputField & reset button doesn't work as expected. #20236

Closed
adsheyn opened this issue Feb 14, 2020 · 7 comments
Labels
[Type] Bug An existing feature does not function as intended

Comments

@adsheyn
Copy link

adsheyn commented Feb 14, 2020

Describe the bug

  1. Input field is still visible when withInputField is set to false.
  2. Adding tick marks like in the example below, doesn't work:
    https://developer.wordpress.org/block-editor/components/range-control/#marks
    No tick marks are displayed.
  3. Input field is not updated when I click on reset button.

Desktop (please complete the following information):

  • OS: iOS
  • Browser chrome v.79
  • Gutenberg 7.4. (also tested with the Gutenberg plugin disabled)
  • WP v.5.3.2
@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Feb 14, 2020
@jorgefilipecosta
Copy link
Member

jorgefilipecosta commented Feb 14, 2020

Hi @adsheyn, thank you for reporting this issue.

Input field is still visible when withInputField is set to false.
Adding tick marks like in the example below, doesn't work

withInputField and Tick marks were only introduced in Gutenberg 7.5 and will not be part of WordPress 5.4, so they don't work in Gutenberg 7.4.

Input field is not updated when I click on reset button.

Until WordPress 5.3 reset was not something automatic. When reset was clicked a call to onChange with an undefined input was done. The component using range control was responsible for handling a call in onChange with an undefined value and resetting the input to its original value. RangeControl can not know what should be set when reset is clicked it depends on the use case.
The reset seems to be working as expected until Gutenberg 7.5.
Possible to test using the block I provided where reset always reverts back to 3.

Testing this, I found that in Gutenberg 7.5 reset tries to set the value back to its original value when the component was mounted.

I think this change should be reverted as it goes against what was documented:

If allowReset is true when onChange is called without any parameter passed it should reset the value.

In WordPress 5.4 beta, if we changed the width of a column and pressed reset, it would always revert to auto. In Gutenberg 7.5, reset reverts to the original value since the component was mounted. It means that if the width was A, and I change it to B when I press reset it goes back to A, If I change it back to B, close the column settings panel, open the columns settings panel, change the value to C and press reset it goes back to B instead of going back to A.

cc: @ItsJonQ I think we may need to change how the reset is handled on the new version of RangeControl as it may break users of the component relying on the documented behavior. E.g: on the test block I provided reset always reverted the value back to 3 and now that is not the case.

Test Block I used (possible to test by pasting this code in the browser console and inserting TestRangeControl block):

( function() {
	var registerBlockType = wp.blocks.registerBlockType;
	var el = wp.element.createElement;
	var useState = wp.element.useState;
	var components = wp.components;
	var InspectorControls = wp.blockEditor.InspectorControls
	registerBlockType( 'test/test-range-control', {
		title: 'TestRangeControl',
		category: 'common',

		edit: function() {
			const [ value, setValue ] = useState( 8 );
			return el( 'div', { className: 'test-single-svg-icon', style: { outline: '1px solid gray', padding: 5 } },
				el( InspectorControls, {},
					el(
						components.RangeControl,
						{
							value,
							label: 'Test',
							min: 1,
							max: 10,
							initialPosition: 5,
							allowReset: true,
							onChange: function( newValue = 3) {
								setValue( newValue );
							},
						}
					)
				)
			);
		},

		save: function() {
			return el(
				'div',
				{ className: 'test-range-control', style: { outline: '1px solid gray', padding: 5 } },
				'Test Block'
			);
		},
	} );
} )();

@ItsJonQ
Copy link

ItsJonQ commented Feb 14, 2020

@jorgefilipecosta Thanks for the heads up + detailed break down! I'll take a look at this right away 🙏

@jorgefilipecosta
Copy link
Member

@ItsJonQ previously I provided the wrong sample, I updated it.
Meanwhile, I noticed another regression initialPosition is being forced, on the sample I provided the value should be 8 at the start, but now we are showing 5. initialPosition should only be used if no value is provided. Another problem seems to be that if no value is provided we render the initialPosition on the input field, I think the idea was that the input field would be empty and the value would just be used to provide a good starting position on the slider.

@ItsJonQ
Copy link

ItsJonQ commented Feb 14, 2020

@jorgefilipecosta Thank you for that info! I just pushed up a PR to hopefully resolve these scenarios!
#20247

@adsheyn
Copy link
Author

adsheyn commented Feb 14, 2020

@jorgefilipecosta Thank you for looking into this issue.
I've upgraded Gutenberg plugin to v.7.5 but unfortunately it didn't solve those problems.
Still can't figure out how to disable the input field or add those tick marks.

EDIT: Now, after updating to 7.5 it is not possible to scroll down the page on mobile devices.
If I deactivate Gutenberg plugin everything works ok.

@simonhammes
Copy link
Contributor

I think the idea was that the input field would be empty and the value would just be used to provide a good starting position on the slider.

@jorgefilipecosta Is this really the expected behavior? Why can't the input field show the actual value 0?

@jorgefilipecosta
Copy link
Member

@jorgefilipecosta Is this really the expected behavior? Why can't the input field show the actual value 0?

Because when no value is provided we don't know the default value (it may be different from 0) e.g: if rangecontrol is used to pick a font size when no value is provided the default font size is not 0 it maybe 12, 14 or even 50, showing empty for defaults seems more accurate as when a value is undefined it may not be 0.

EDIT: Now, after updating to 7.5 it is not possible to scroll down the page on mobile devices.
If I deactivate Gutenberg plugin everything works ok.

This was the result of another issue that is already fixed. If the problem is still happening in the last version please leave a comment and we will debug the issue further. Regarding the original issue, it also seems fixed so I'm going to close the issue for now.

Thank you all for the insights provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants