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

Stepped Slider: Followup changes and improvements #43412

Open
5 of 7 tasks
jasmussen opened this issue Aug 19, 2022 · 19 comments
Open
5 of 7 tasks

Stepped Slider: Followup changes and improvements #43412

jasmussen opened this issue Aug 19, 2022 · 19 comments
Assignees
Labels
[Feature] UI Components Impacts or related to the UI component system Needs Dev Ready for, and needs developer efforts Needs Figma Update Needs an update to Figma for design purposes [Type] Enhancement A suggestion for improvement.

Comments

@jasmussen
Copy link
Contributor

jasmussen commented Aug 19, 2022

A stepped slider recently landed, allowing you to set padding in multiples of a base unit. There are a few followups to look at.

Fix artifact glitches 🚨

  • Fix artifact glitches with knob

Occasionally (exact steps to reproduce are unclear) but there's occasionally a white artifact on top of the blue knob:

Screenshot 2022-08-19 at 10 44 47

Update the design to remove the knob

  • Move to a segment selection interface instead of the knob

Placing the handle at the end of the current value makes sense when we think of this as a traditional range, but it might be better for each step to act as a handle:

This makes it a bit more obvious that each step acts as an individual button for quicker jumping.

Increase density when unlinked

The unlinked splits each value into a single row which occupies a lot of space in the context of other controls. We can remedy this by splitting the layout into two columns:

Screenshot 2023-02-17 at 10 32 47

Nice to have: Unit multiplier in the padding visualization

  • Show the unit multiplier in the padding visualization, top/right/bottom/left on the padding silhouette

Screenshot 2022-09-16 at 16 05 43


Issue updated Nov 21.

Completed

Replace the labels with numbers

The values 0-7 replace 0, 2x-small, x-small, small, medium, large, x-large, 2x-large

Spacing

Update spacing and labels

@jasmussen jasmussen added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system labels Aug 19, 2022
@jameskoster

This comment was marked as resolved.

@jameskoster

This comment was marked as resolved.

@glendaviesnz
Copy link
Contributor

Don’t show a value label, but show the multiplication operation we’re doing. Don’t include the unit (rem) — just the increment. The actual multiplication.

Just a note that we would need to do some detection to work out what to show here, as it is possible for theme authors to add a static list of values that aren't calculated, in which case we would have to default back to the increment names.

As @jameskoster notes it is also possible to use + increment instead of a * so would need to work out how to indicate that as well. I don't think 1,2,3,4,5 alone is a great option - it didn't seem to be well accepted on the font size control which has gone back to t-shirt sizes ... which maybe is a reason to stick with the t-shirt sizes here for consistency?

@jasmussen
Copy link
Contributor Author

Just a note that we would need to do some detection to work out what to show here, as it is possible for theme authors to add a static list of values that aren't calculated, in which case we would have to default back to the increment names.

Should this even be allowed? It seems like in these cases we should just fall back to the custom value control.

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Aug 22, 2022

Should this even be allowed?

Yep, absolutely - there was discussion with theme authors about this here - actual comments may be collapsed now, but there was a clear indication from them that they wanted the flexibility to define their own custom list of presets that may or may not consist of a calculable scale (eg. some are already using there own set list in order to support fluid clamp values which are not currently supported in the UI). A custom scale fall back is not an alternative in their case as they want to limit their users to a set list of options.

@jasmussen
Copy link
Contributor Author

jasmussen commented Aug 22, 2022

Right, makes sense, but I could see that being a dropdown list like it is for font sizes. The segmented slider to me indicates a clear delineation of values, and that assigning random properties on such a linear scale is likely going to confuse. A dropdown fallback is something we can explore, but it doesn't seem urgent to optimize the stepped slider for arbitrary values.

@glendaviesnz
Copy link
Contributor

Yeh, for sure, not a blocker to looking at improvements, was just pointing it out so it was not overlooked as a requirement.

@aaronrobertshaw
Copy link
Contributor

Thanks for exploring these follow-ups 👍

There's another aspect to this control that I think we could explore improving: the UI's complexity when manipulating all sides.

It wasn't too long ago we were undertaking a real push to clean up the sidebar and reduce clutter. Even with only the padding control in an expanded state, the dimensions panel gets very busy quickly. Once we also have margin support, that problem is at least doubled. This is before blocks begin to add further controls there such as height, width etc.

Is it worth exploring using a popover for the expanded/unlinked view?

Screen Shot 2022-08-22 at 4 48 19 pm

@jasmussen
Copy link
Contributor Author

Yes! 100% that should be improved. It deserves some good ideas with mockups, plus its own ticket. A few ideas to consider for precedence, such as the border control:

Screenshot 2022-08-22 at 10 10 52

Or perhaps even a more basic layout a la Figma:

figma

This older (outdated) mockup splits it more simply, like so:

Padding

In all these cases, the stepped slider by virtue of its size may need to be invoked in a popover. But could we start with a stepper?

@ndiego
Copy link
Member

ndiego commented Aug 22, 2022

Hi all, I have a question about the actual variable names that are calculated. Perhaps I missed a previous comment, but it's a bit weird why the variables are 30, 40, 50, etc. when that does not match the actual value. See the screenshot below. Maybe something more generic, like 1, 2, 3, 4, etc? I do like how the variables are not tied to the value, as this allows you to do some cool things based on device size.

image

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Aug 22, 2022

but it's a bit weird why the variables are 30, 40, 50, etc.

@ndiego there was a lot of discussion around this here (some of the comments around it are probably hidden in that thread now as it was getting quite long). The choice of these slugs was based around wanting to make it easier for themes to insert additional options between the Core provided values, eg if a theme wants an extra value between 30 and 40 they can add 35 - which is better than 3.5. The choice of numbers here was also to allow the ability to easily fallback to nearest option if block content/patterns are copied between themes that don't support the same list of presets, eg. --wp--preset--spacing-35 can be mapped to --wp--preset--spacing-30.

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Aug 23, 2022

eg if a theme wants an extra value between 30 and 40 they can add 35

Except I have just noticed that values are merged into the Core settings for the generation of the CSS properties, but not at the UI level for the selection of the values, so need to take a look at that. fontSizes does the same, merges the theme and core values to produce the presets CSS properties, but then only shows the theme values to select from in the UI - not sure what the thinking is behind this 🤔

@jasmussen

This comment was marked as resolved.

@priethor priethor added the [Priority] High Used to indicate top priority items that need quick attention label Sep 16, 2022
@glendaviesnz
Copy link
Contributor

glendaviesnz commented Sep 19, 2022

Extracting from a conversation here, let’s move forward with the numbers as spacing indicators rather than "1x", or the current "small/medium/large" etc.

@jasmussen there is a PR that implements this here.

This is mainly important for any CSS classes we mean to extrapolate from the value, which should be along the lines of has-margin-s1 or has-spacing-1, rather than has-spacing-small.

Just to clarify, there are currently no CSS classes as part of the first release of the spacing presets in 6.1, they are all applied via inline styles and CSS vars, and these are extrapolated from the slug value, not the name/label that appears in the UI. As noted here the slugs are of the format 10,20,30,40.

The spacingSizes are generated up and down from a midpoint of 50 in order to allow for better cross-site/theme fallbacks if copied blocks or patterns use spacing preset values not supported by the current theme, ie. if different spacing scales all have a common medium point in the scale we are more likely to get a comparable match when content is moved between sites. The one drawback of switching from t-shirt sizing to digits is that there now appears to be a vague correlation between the label and the slug, eg. on this PR if you set a value of 1 you will get a CSS var of var(--wp--preset--spacing--20) applied as the default Core scale only has 3 steps either side of the 50 medium step. This may or may not be a problem, but could potentially be slightly more confusing than a 2x-small label and 20 slug combination where there is no correlation between the label and slug.

We can't add CSS classes until we have the infrastructure in place to apply them dynamically on the front end based on the current blocks as there need to be a huge number of them, eg. has-spacing doesn't work as there need to be classes for each spacing type, eg. has-margin, has-padding, etc. and for each side, has-margin-left, has-margin-right and for each spacing size, has-margin-left-10, has-margin-left-20, etc.

@glendaviesnz
Copy link
Contributor

I think everything that was planned for 6.1 is done now, so removing from the 6.1 editor board

@michalczaplinski michalczaplinski removed their assignment Oct 18, 2022
@jameskoster
Copy link
Contributor

Placing the handle at the end of the current value makes sense when we think of this as a traditional range, but it might be better for each step to act as a handle:

This makes it a bit more obvious that each step acts as an individual button for quicker jumping.

@jasmussen
Copy link
Contributor Author

I updated the issue to include that mockup, Jay!

@jasmussen
Copy link
Contributor Author

I've conditionally checked the box for the visual glitch when dragging in Safari, mainly because I can no longer reproduce it in trunk. If you can, if you still see that white glitch, please do uncheck the box again and ideally share reproduction notesd, thank you!

@annezazu annezazu removed the [Priority] High Used to indicate top priority items that need quick attention label Dec 19, 2023
@annezazu
Copy link
Contributor

👋🏼 I'm doing a sweep of high priority labeled issues and noticed this has greatly stalled. This label is meant for high priority items that require quick attention to resolve. To keep this label actionable and high impact, I'm culling some of the older, stalled items so we can revisit. If you believe the label should be added back, feel free to do so/just let me know and let's do what we can to mobilize efforts. As it stands though, this isn't on the 6.5 roadmap and doesn't reflect current high priority items from what I can see across the project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system Needs Dev Ready for, and needs developer efforts Needs Figma Update Needs an update to Figma for design purposes [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

9 participants