RangeControl: Update the default marks styles to match the padding/margin control #67611
+19
−49
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Feature] UI Components
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -197 B (-0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 25679e2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12184741165
|
|
I would support unifying on the left one for the immediate future, and then keep exploring visual enhancements to that, for example those tracked in #43412. But I believe @jameskoster had some designs too, so would love his thoughts. |
ciampo
reviewed
Dec 5, 2024
Comment on lines
+206
to
+208
| @media not ( prefers-reduced-motion ) { | ||
| transition: left ease 0.1s; | ||
| } |
There was a problem hiding this comment.
This animation introduces a subtle delay when dragging the thumb that can make the UI feel unresponsive. We could consider removing it for the time being, and focus this PR on updating the marks design.
(also, in the block editor "custom" version, this animation was only applied when the is-marked class is applied — just noting in case that was deliberate)
There was a problem hiding this comment.
The animation already exists in padding/margin so removing it could be seen as a regression in the behavior of the component.
There was a problem hiding this comment.
@jasmussen should we remove the animation or keep it?
There was a problem hiding this comment.
Here's the animation, though in low framerate GIF:
Note the slight disconnect with the cursor.
Here's with no animation:
It's not a huge difference, though I tend to think with animation is preferred, as it make it "snap" into place a bit more.
There was a problem hiding this comment.
Got it — I will then propose that we keep the animation only when the component is showing marks (like it was in the deleted code from packages/block-editor/src/components/spacing-sizes-control/style.scss). I can open a PR for that
Comment on lines
+133
to
+135
| @media not ( prefers-reduced-motion ) { | ||
| transition: width ease 0.1s; | ||
| } |
There was a problem hiding this comment.
It looks like this animation is there to go hand in hand with the thumb's animation — we should remove it if we remove the thumb's animation.
(sidenote: animating the width is not very performant, and can cause the UI to feel sluggish instead of polishing the animation).
If were to keep the animation around, we could add a comment to help future readers understand that the track and thumb animations go hand in hand.
No, they'll just inherit the new styles (they were using the old ones) |
mirka
reviewed
Dec 5, 2024
There was a problem hiding this comment.
The mark labels feel a bit far now, how about something like 8px here:
diff --git a/packages/components/src/range-control/styles/range-control-styles.ts b/packages/components/src/range-control/styles/range-control-styles.ts
index 638ccde548..449ea488d2 100644
--- a/packages/components/src/range-control/styles/range-control-styles.ts
+++ b/packages/components/src/range-control/styles/range-control-styles.ts
@@ -167,7 +167,7 @@ export const MarkLabel = styled.span`
color: ${ COLORS.gray[ 300 ] };
font-size: 11px;
position: absolute;
- top: 22px;
+ top: 8px;
white-space: nowrap;
${ rtl( { left: 0 } ) };| Before | After |
|---|---|
 |
 |
packages/components/src/range-control/styles/range-control-styles.ts
Outdated
Show resolved
Hide resolved
I think those are bigger changes that should be explored separately. Looking back I worry the design diverges from traditional range appearance a bit too much. |
ntsekouras
approved these changes
Dec 6, 2024
There was a problem hiding this comment.
LGTM, thanks!
Let's ship unless I misunderstood @jameskoster comment about this PR. What I got from that comment is the he's concerned about the suggestions of this issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discovered in #67544 (comment)
What?
Right now, in the code base we have two different styles for the range control
It seems the second one is kind of a random unintended addition and that we always wanted the former to be the default style for the marks. This PR updates the default style and removes the overrides.
Testing Instructions
This is used in only two places in Gutenberg right now, you can check both of them: