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

Used fallBack styles to compute font size slider initial value. #6088

Merged
merged 1 commit into from
Apr 11, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Apr 9, 2018

Description

This PR changes the RangeSlider to have a new prop called initialPosition. If this prop is set, when no value is passed to the slider, the slider appears with the given start value.
withFallbackStyles is used to get the font size of the paragraph if it is not known. Fallback value is used as the initial position in the slider.
As we use compute styles, even if the paragraph is nested and a CSS rule changes the size of the nested paragraph, the default value in the slider should be right.

Before the slider was in the middle so when the user changed the slider it would make the size very big and different from what it was. Now the slider starts at the correct size so when the user slides the behavior looks more correct.

How Has This Been Tested?

Add paragraph verify the slider starting position corresponds to the actual font size of the paragraph.

Screenshots (jpeg or gifs if applicable):

After:
apr-09-2018 18-31-11
Before:
apr-09-2018 18-38-12

@jorgefilipecosta jorgefilipecosta self-assigned this Apr 9, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the update/fontSize-slider-initial-value branch from 037e1f5 to d44f39d Compare April 9, 2018 18:07
@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. labels Apr 10, 2018
@jasmussen
Copy link
Contributor

Love this. Works as advertised. 👍 👍

For context, this was my only remaining issue with using the slider for the font size — that the initial state was just "centered". It didn't properly communicate "unset", and it was just jarring when you started dragging and you'd immediately jump to a giant size. Nice work.

@gziolo
Copy link
Member

gziolo commented Apr 10, 2018

It all works pretty well I found one small issue. When using Reset button, it doesn't always return to the initial value. We probably need to ensure it is never updated in the state.

Code wise everything looks good aside from this little thing. I hope it isn't hard to fix.

@jorgefilipecosta jorgefilipecosta force-pushed the update/fontSize-slider-initial-value branch from d44f39d to b6aa486 Compare April 10, 2018 15:46
@jorgefilipecosta
Copy link
Member Author

Hi @jasmussen and @gziolo thank you for the reviews.

When using Reset button, it doesn't always return to the initial value. We probably need to ensure it is never updated in the state.

Nice catch, I was able to reproduce the behavior when the paragraph was already saved with a specific size doing reset was not moving the slider, I applied a fix and I think the problem is now solved.

@gziolo gziolo added this to the 2.7 milestone Apr 11, 2018
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Everything works perfectly fine with the recent code update. Thanks for solving this subtle but very annoying UX issue 👏

@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Apr 11, 2018
@jorgefilipecosta jorgefilipecosta merged commit bd55363 into master Apr 11, 2018
@jorgefilipecosta jorgefilipecosta deleted the update/fontSize-slider-initial-value branch April 11, 2018 10:00
@aduth
Copy link
Member

aduth commented Apr 18, 2018

Should this have been called defaultValue for consistency with React?

https://reactjs.org/docs/uncontrolled-components.html#default-values

@jorgefilipecosta
Copy link
Member Author

Hi @aduth,

Should this have been called defaultValue for consistency with React?

The RangeControl has "two inputs" a slider and an input text with the value. The initialPosition prop is just used to position the slider in a given place when no value was set. It is not exactly a default value as for example the input text still shows no value. Using defaultValue may create the expectation that we are in the presence of a normal defaultValue.

@aduth
Copy link
Member

aduth commented Apr 18, 2018

Okay, that makes sense. It wasn't entirely clear from the documentation that this is more of a visual effect, not an actual value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Needs Design Feedback Needs general design feedback. [Type] Bug An existing feature does not function as intended [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants