Update: Make changing order an action on the ellipsis menu. #62189
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
|
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. |
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
2 times, most recently
from
34b4555 to
a9732b1
Compare
|
Size Change: +156 B (+0.01%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
from
4b08c57 to
8616147
Compare
|
Flaky tests detected in 34b1b2d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9352601268
|
|
Thanks @jorgefilipecosta. I feel this better manages the prominence of this relatively obscure attribute. In the future it would be good to hide it altogether in favor of more intuitive experiences. One small tweak; can we use the |
|
Hey @jorgefilipecosta |
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
2 times, most recently
from
9a44348 to
34b1b2d
Compare
Hi @jameskoster, thank you for reviewing this change, the small variant size was applied, let me know it it looks as expected. |
Hi @paaljoachim, the before image was added. |
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
from
34b1b2d to
9798852
Compare
|
Ahhh.
That means that a shift has happened between what I see in the screenshot that I shared and the above example Jorge shared. Moving the Reorder into the ellipsis menu seems like a good way to shorten down the long list of information seen. |
|
Hi @jameskoster, can you have another look and see if it already matches what is expected? |
|
It's super obscure, and not helpful for most implementations of WordPress (any time you have a custom menu). 😅 |
richtabor
reviewed
Jun 6, 2024
| /> | ||
| <div> | ||
| { __( | ||
| 'This attribute determines the order of pages in the Pages List block.' |
There was a problem hiding this comment.
Let's just make this one paragraph, with the following:
"Determines the order of pages in the Page List block. Pages with the same order value are sorted alphabetically. Negative order values are supported."
There was a problem hiding this comment.
Hi @richtabor your feedback was applied, let me know if it is ok to merge this change.
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
from
215bae1 to
143d127
Compare
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
from
143d127 to
5ae2661
Compare
t-hamano
reviewed
Jun 19, 2024
There was a problem hiding this comment.
I have made some suggestions, but I would be glad to hear @richtabor's feedback again regarding the text changes.
Comment on lines
713
to
686
| style={ { | ||
| // editor styles are not loaded in dataviews so we use an inline style here. | ||
| maxWidth: 384 - 32 - 32 - 6 - 6, | ||
| } } |
There was a problem hiding this comment.
Is this style necessary? It seems to make the content width 372px instead of the expected 384px ($modal-width-small).
There was a problem hiding this comment.
This style causes form elements to shift in width.
| With maxWidth | Without maxWidth |
|---|---|
 |
 |
richtabor
reviewed
Jul 1, 2024
| /> | ||
| <div> | ||
| { __( | ||
| 'This attribute determines the order of pages in the Pages List block.' |
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
2 times, most recently
from
a398945 to
4c0b0d7
Compare
Hi @richtabor, |
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
from
4c0b0d7 to
5d80bb2
Compare
I'm worried that this specification may cause accessibility problems.
The video below shows how with NDVA enabled, the screen reader loses focus when the order number is reset to zero on the keyboard. d6a2984e3f78a5febe942dbd0ebb5f12.mp4cc @richtabor |
|
Order is such an obscure attribute, and so rarely used, I don't know that we ever need to display it directly in the sidebar. If you're familiar with the feature, you'll understand that any non-zero value = customized. |
In my opinion, there are two possible approaches to get around this problem:
Do you have any good ideas? |
|
I guess we can go for "Move the Order action to the elipsis menu and NOT display it in the summary panel". @richtabor @WordPress/gutenberg-design would that change be ok? |
|
That approach would get my vote :) |
I suppose that's the better of the two options. |
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
from
5d80bb2 to
a5898f0
Compare
|
Thank you all for the insights I rebased the PR and applied the recommended path. I think this is ready for a last checkup and merge. cc: @jameskoster |
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
from
60bb53d to
d7765df
Compare
jameskoster
approved these changes
Jul 16, 2024
t-hamano
approved these changes
Jul 16, 2024
Comment on lines
713
to
686
| style={ { | ||
| // editor styles are not loaded in dataviews so we use an inline style here. | ||
| maxWidth: 384 - 32 - 32 - 6 - 6, | ||
| } } |
There was a problem hiding this comment.
This style causes form elements to shift in width.
| With maxWidth | Without maxWidth |
|---|---|
|
|
Comment on lines
5
to
7
| .editor-action-modal__reorder-pages .components-modal__frame { | ||
| max-width: $modal-width-small; | ||
| } |
There was a problem hiding this comment.
There is no element with this selector and it would be unnecessary.
There was a problem hiding this comment.
Good catch it was removed 👍
jorgefilipecosta
force-pushed
the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
from
2e985e8 to
b56cbde
Compare
Co-authored-by: Aki Hamano <[email protected]>
This reverts commit 33d0bbf.
Co-authored-by: Aki Hamano <[email protected]>
jorgefilipecosta
deleted the
update/make-changing-order-an-action-on-the-elipsis-menu
branch
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
…s#62189) Co-authored-by: Aki Hamano <[email protected]>
priethor
added
the
[Feature] DataViews
14 tasks
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 implements a pending change suggested by @jameskoster at #61918 (comment) to make "reorder" an action on the ellipsis menu instead of an item on the page summary.
Screenshots
Before:
After:
Testing Instructions
Verified I could correctly reorder a page on the post editor, site editor, and pages list on dataviews.