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

BoxControl: Add support for presets #67688

Merged
merged 7 commits into from
Dec 12, 2024
Merged

BoxControl: Add support for presets #67688

merged 7 commits into from
Dec 12, 2024

Conversation

youknowriad
Copy link
Contributor

Related #67625

What?

This PR implements "presets" support in the BoxControl component (borrowing the behavior from the spacing sizes with some subtle differences)

  • When the presets are provided, there's a button to switch between a custom value and a preset value.
  • A range control is used always for presets (this is to allow iteration, in the next PR I'll add a Select control for long presets)
  • If I switch back and forth between presets and custom value, I don't try to "guess" the value from the preset or the opposite, this is a conscious decision because for me, setting a preset who's current value is 4px is different than setting 4px custom value. The preset value can change over time.
  • I added a storybook story to try this

Testing Instructions

  • Run storybook locally npm run storybook:dev
  • You can check the "BoxControl with presets" story.

@youknowriad youknowriad added [Feature] UI Components Impacts or related to the UI component system [Type] New API New API to be used by plugin developers or package users. labels Dec 6, 2024
@youknowriad youknowriad self-assigned this Dec 6, 2024
@youknowriad youknowriad force-pushed the update/refactor-box-control branch 2 times, most recently from f15066c to 5c8d201 Compare December 9, 2024 16:19
Base automatically changed from update/refactor-box-control to trunk December 10, 2024 09:25
@youknowriad youknowriad marked this pull request as ready for review December 10, 2024 10:01
Copy link

github-actions bot commented Dec 10, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

github-actions bot commented Dec 10, 2024

Flaky tests detected in a0c5d29.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12294409796
📝 Reported issues:

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

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

This looks good in general. I've left just a few small suggestions/questions. Thanks!

@tyxla tyxla requested a review from a team December 11, 2024 09:31
@youknowriad youknowriad requested a review from Mamaduka December 11, 2024 12:31
@jorgefilipecosta
Copy link
Member

When I select a preset e.g: Medium and then switch to "Set custom size" I would expect the field to have the value of the preset (8px), but it becomes empty.

I disagree and I explained this in the description as well. Choosing a preset is different than setting a custom value. The present is a dynamic value that changes over time, so the user should be able to set a specific 8px or a dynamic preset whose current value is 8px and these two things are not the same.

I understand your point. However, I believe that when a preset is selected and then the user switches to the custom option, the current value should still display as 8px, even though a preset is being used. This would help users see the value that is currently applied, making it easier to make smaller adjustments. Without this visibility, I might struggle to decrease the margin slightly since I wouldn’t know the exact value set with the presets, and there is no easy way to just decrease 1px.

We could differentiate the handling of values by storing a different value when 8px is chosen as a custom value versus when it is selected as a preset. Although they may appear the same, they should be stored differently to reflect user intent. I think this solution addresses your concern.

We also need to decide whether switching to custom should trigger an onchange event that changes the displayed value from the preset notation to the custom notation (which includes the value). Alternatively, we could show the custom value but keep it stored as a preset until the user makes additional adjustments. I am fine with either option.

This discussion is connected to the effort of displaying global styles values at the block level. We aim to show these values even if the block itself has no styles applied and only inherits them from global styles.

This is a personal preference and not a blocker for the pull request.

@youknowriad
Copy link
Contributor Author

I understand your point. However, I believe that when a preset is selected and then the user switches to the custom option, the current value should still display as 8px, even though a preset is being used. This would help users see the value that is currently applied, making it easier to make smaller adjustments. Without this visibility, I might struggle to decrease the margin slightly since I wouldn’t know the exact value set with the presets, and there is no easy way to just decrease 1px.

Do you think the user would understand that "touching" the field from 8px and just removing and adding the same character back switches from "preset value" to "custom value"? I think that might be more confusing.

I agree though that both options have downsides, although both options are possible. I think right now in this PR, we're just adding support for "presets" to a component that is not used, so this is not going to have an impact immediately but I think we should keep this discussion open with designers as well @jasmussen @jameskoster. Especially, in my follow-up PRs where I'll be replacing the custom components with BoxControl

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

This is working well, and @youknowriad clarified the concerns I had 👍

@jameskoster
Copy link
Contributor

From my experience, the behavior Jorge describes is quite common, particularly when detaching from presets. Here's an example from Figma that I use all the time:

detach.mp4

Agree there are trade-offs both ways, but this approach feels a bit more standard to me.

@youknowriad
Copy link
Contributor Author

@jameskoster I agree that with the "detach" button, the behavior you're showing the best one. I think we can explore the detach button separately but right now it doesn't exist anywhere in the UI AFAIK

@youknowriad youknowriad merged commit 4263e85 into trunk Dec 12, 2024
65 checks passed
@youknowriad youknowriad deleted the add/presets-support branch December 12, 2024 16:43
@github-actions github-actions bot added this to the Gutenberg 20.0 milestone Dec 12, 2024
@jameskoster
Copy link
Contributor

Yes fair enough. I guess it's a question of whether the button is about detaching or setting a custom value. Personally I think detaching has it's uses–many of which Jorge outlined above–and is a precursor to setting a custom value anyway. Though I appreciate the existing UI focuses more on the latter.

I'll open an issue.

@jameskoster
Copy link
Contributor

Actually @youknowriad it does exists 🙊

size.mp4

Notice how toggling to set a custom value after selecting a preset font size, the size value/unit remains.

@youknowriad
Copy link
Contributor Author

@jameskoster you misunderstood what I'm saying, Yes, we do keep the value visible but we don't have the "unlink" button which means there's no way for the user to understand whether the value that is visible in the custom input is actually "linked" or not "linked" to the preset.

So in the screenshot you shared, the value is "linked" but the user doesn't know it, the only way to "unlink" it but keep the same value is to actually type some random character in the input and remove it.

@jameskoster
Copy link
Contributor

Oh wow, I see what you mean. TIL that switching to set a custom value doesn't unlink from the preset until you also change the value. I guess it's a small detail but this behavior feels unexpected to me.

@ciampo
Copy link
Contributor

ciampo commented Dec 13, 2024

A bit late to the conversation, but my preference would be:

  • when moving from preset to custom, the value is not lost. But the component should know the difference between "8px as a preset value" and "8px as a custom value"
  • when moving from a custom value to a preset:
    • if the custom value matches one of the presets, such preset gets selected
    • if there isn't a custom match, we could display a literal "custom" option, until the user picks a preset

The FontSizePicker already does something similar when displaying a SelectControl instead of a ToggleGroupControl where there are many font sizes:

Kapture.2024-12-13.at.16.42.10.mp4

In any case, whatever UX we think is the best, we should apply uniformly across all UI components

### `presetKey`

The key of the preset to apply.
If you provide a list of presets, you must provide a preset key to use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Potential improvements for Developer Experience:

  • a runtime check, warning when only one of the two presets and presetKey props are defined
  • conditional types, specifying that presetKey is not optional if the presets prop is specified

yogeshbhutkar pushed a commit to yogeshbhutkar/gutenberg that referenced this pull request Dec 18, 2024
Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>
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 Accessibility Feedback Need input from accessibility [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants