Update: Move item size control to the new view config UI. #64380
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
jorgefilipecosta
added
[Type] Enhancement
|
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. |
|
I wonder if instead of the eye icon we should try a check-box based interface. The eye doesn't feel super clear here. The closed eye also isn't super clear, I wonder if we shouldn't refresh it to be the eye but with a diagonal line across. |
Happy to try that separately, I don't think that icon exists in the set yet. @jorgefilipecosta thanks for the work so far, a couple of small suggestions:
|
jorgefilipecosta
force-pushed
the
update/move-item-size-control-to-the-new-view-config-ui
branch
from
f8e9334
to
5ff17de
Compare
Hi @jasmussen, seems like something we can try, I can have a look in a different PR. @jameskoster all your feedback was applied. |
|
Size Change: -225 B (-0.01%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Thanks @jorgefilipecosta, I'm seeing two small issues... Sometimes I'm unable select the last value in the range, here's a video to demonstrate: bug.mp4Probably separate but the appearance of the marks is a bit buggy. I think they should rest on top of the track rather than above it, something like this: 
Maybe @WordPress/gutenberg-components has some insights there. |
|
Hi @jameskoster, the bug was fixed 👍 Regarding the marks I agree that your proposal seems like a preferred behavior, but I think we can fix it outside of this PR. The fix is on the component is even on the stories https://wordpress.github.io/gutenberg/?path=/story/components-rangecontrol--with-integer-step-and-marks it shows the UI as in this PR. |
|
Thanks for the fix, it's working now :) For the marks I suppose the question is whether or not this PR should be blocked by the visual bug. I suppose the other option would be to remove them for now and open a follow-up later once the display issue has been fixed. What do you think? |
Looks unrelated to this PR. I can repro in Storybook:
@mirka could this be a recent regression caused by any of our work on #38730? |
No, I think the positioning of the marks was weird like this, for as long as I can remember 😅 I don't even know what the intended design was, but it does look like it was vertically centered on the line, originally (#19916). We should definitely fix it — I assumed it was an outdated feature or something because we never really heard about the visual issue. |
|
It's complicated further by I think the way to proceed here is;
|
jorgefilipecosta
force-pushed
the
update/move-item-size-control-to-the-new-view-config-ui
branch
from
975f6c3
to
4f1673a
Compare
@jameskoster, Sounds like a good plan, following it, I removed the marks. |
jameskoster
approved these changes
Aug 13, 2024
jameskoster
previously requested changes
Aug 13, 2024
Hi @jameskoster, I missed a commit push, it should be fixed. |
|
Forgive me for jumping in randomly with a possibly context-lacking question: can this use the same design as the existing stepped slider we have? |
t-hamano
reviewed
Aug 14, 2024
| __nextHasNoMarginBottom | ||
| __next40pxDefaultSize | ||
| showTooltip={ false } | ||
| className="dataviews-density-picker__range-control" |
There was a problem hiding this comment.
This selector is no longer used anywhere so I think it can be removed.
|
Good find @t-hamano, the sorting issue is not related to this PR. It only affects patterns (not templates and pages). I will look for a fix and propose a PR to it. |
Cool, well it seems like @jameskoster opened an issue already (#64481) and @ramonjd already opened a PR to fix it (#64487) 👏 - let's make sure to review that. |
@jasmussen I think you're referring to @jorgefilipecosta it seems the marks issue on Otherwise this seems good to me, pending resolution to Aki's feedback. |
jorgefilipecosta
force-pushed
the
update/move-item-size-control-to-the-new-view-config-ui
branch
from
5ceb9ec
to
2c8d3d4
Compare
jorgefilipecosta
deleted the
update/move-item-size-control-to-the-new-view-config-ui
branch
|
Merging this one as all the blockers were addressed will follow up with the addition of the marks. |
artemiomorales
added
[Feature] Data Views
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.
This PR moves the item size picker to the new view configuration UI.
Follow up to #64175.
cc: @WordPress/gutenberg-design
Screenshots
Testing Instructions
I verified the item size picker is only available on the grid layout and still works as expected.