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

Fix RangeControl mark placement and cursor styles #26745

Merged
merged 4 commits into from
Nov 23, 2020

Conversation

stokesman
Copy link
Contributor

@stokesman stokesman commented Nov 5, 2020

RangeControl has the little-used feature of marks. Using the prop marks they can be placed automatically or specified ("custom" in the screenshots). The placement of specified marks will be incorrect if the step property is decimal or the min property is negative.

An example with step property as decimal:

Before After
range-controls-decimal-step range-controls-fixed-decimal-step

An example with min={ -10 } and max={ 10 }:

Before After
range-controls-negative-min range-controls-fixed-negative-min

In such cases not only are the custom marks out of place, (when value is undefined) the calculation for which marks are represented as "filled" is incorrect.

Additional changes not related to the aforementioned mark issues are minor styling changes to fix: #25345. Though it turns out those styling changes helped reveal a further issue with the marks. They obscure interaction with the input:
range-control-mark-pointer
Not something that would occur often but glad to have caught it. Commit 8d8b31a1d9f0565dc674fd6ced219e7f94374aaf pushed to resolve that.

How has this been tested?

In Storybook using this story:
https://gist.github.com/stokesman/155ec2791493a3e9fe134ac6ee988235
In WordPress 5.5.1 with a custom block using a RangeControl with custom marks

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.

@stokesman
Copy link
Contributor Author

While looking for existing issues I found these that appear outdated and able to be closed #12429 and #12432.

@stokesman stokesman force-pushed the fix/range-control-custom-marks branch from 8d8b31a to b17b287 Compare November 9, 2020 17:08
@stokesman stokesman marked this pull request as ready for review November 9, 2020 18:46
@stokesman
Copy link
Contributor Author

@ItsJonQ, it'd be great if you can have a look (at your leisure, I don't think there's any urgency).

Currently, in Gutenberg, there are no instances of the use cases fixed by this PR. So for testing Storybook is easiest (but also lacks coverage of such use cases). This gist contains the story I used in testing. If it's deemed worthwhile I'm willing to add stories to this PR, but I'd like to know if there are some guidelines to follow for those additions.

@ItsJonQ ItsJonQ self-requested a review November 12, 2020 15:08
@ItsJonQ ItsJonQ added the [Package] Components /packages/components label Nov 12, 2020
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.

@stokesman 🚀 from me! Thank you so much for the update! You're totally right. The previous implementation didn't correctly handle negative min values. The pointer-events fix is also very welcomed.

The gist you shared was very helpful! Do you think you'd be able to add that to the existing story? :)

I think it would be helpful for future testing and development.

Thanks! ✨

@stokesman stokesman force-pushed the fix/range-control-custom-marks branch from 730a8c3 to 1564533 Compare November 23, 2020 06:08
@ItsJonQ ItsJonQ merged commit 10c48e1 into WordPress:master Nov 23, 2020
@github-actions github-actions bot added this to the Gutenberg 9.5 milestone Nov 23, 2020
@stokesman stokesman deleted the fix/range-control-custom-marks branch November 24, 2020 03:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RangeControl styling sins
2 participants