DataViews: make it the root for pages and add details #57578
Merged
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
oandregal
requested review from
youknowriad,
jameskoster,
jorgefilipecosta and
ntsekouras
oandregal
added
[Type] Experimental
|
Size Change: +600 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
oandregal
commented
Jan 5, 2024
packages/dataviews/src/view-list.js
Outdated
| @@ -89,6 +93,19 @@ export default function ViewList( { | |||
| </div> | |||
| </VStack> | |||
| </HStack> | |||
| { onDetailsChange && ( | |||
| <Button | |||
There was a problem hiding this comment.
I'm looking at how to make this button appear visually within the other interactive element (<div role="button">) while avoid nesting in the DOM.
There was a problem hiding this comment.
I played with the idea of having two buttons: one nested and hidden from the accessibility tree, the second not nested and visually hidden.
It works, as demonstrated in the accessibility tree. Though, I'm not super keen on this idea:
Gravacao.do.ecra.2024-01-05.as.12.23.16.mov
In terms of code, something along these lines could work. It'd need factoring into a single component.
diff --git a/packages/dataviews/src/style.scss b/packages/dataviews/src/style.scss
index 93926db47d..72037da3d6 100644
--- a/packages/dataviews/src/style.scss
+++ b/packages/dataviews/src/style.scss
@@ -280,6 +280,16 @@
.dataviews-list-view__details-button {
align-self: center;
}
+ .dataviews-list-view__details-button-visually-hidden {
+ position: absolute;
+ width: 1px;
+ height: 1px;
+ margin: -1px;
+ padding: 0;
+ overflow: hidden;
+ clip: rect(0, 0, 0, 0);
+ border: 0;
+ }
}
.dataviews-action-modal {
diff --git a/packages/dataviews/src/view-list.js b/packages/dataviews/src/view-list.js
index cf9ca31851..ccfc821956 100644
--- a/packages/dataviews/src/view-list.js
+++ b/packages/dataviews/src/view-list.js
@@ -95,6 +95,7 @@ export default function ViewList( {
</HStack>
{ onDetailsChange && (
<Button
+ aria-hidden={true}
className="dataviews-list-view__details-button"
onClick={ () =>
onDetailsChange( [ item ] )
@@ -108,6 +109,19 @@ export default function ViewList( {
) }
</HStack>
</div>
+ { onDetailsChange && (
+ <Button
+ className="dataviews-list-view__details-button-visually-hidden"
+ onClick={ () =>
+ onDetailsChange( [ item ] )
+ }
+ icon={
+ isRTL() ? chevronLeft : chevronRight
+ }
+ label={ __( 'View details' ) }
+ size="compact"
+ />
+ ) }
</li>
);
} ) }
There was a problem hiding this comment.
I've also played with them being siblings within the same HStack. Tried different approaches, but wasn't able to make the first interactive element take all the space up to the second. There's always a weird gap.
Even if we were able to remove that gap and the button take the full height, visually, they will appear as two consecutive elements (see focus). Would that be a fine trade-off design-wise @jameskoster ?
Gravacao.do.ecra.2024-01-05.as.12.30.28.mov
There was a problem hiding this comment.
Instead of duplicating 'details' buttons, could the main button and details button be adjacent, with the latter positioned absolutely? Something like this codepen.
There was a problem hiding this comment.
Related, if the entire element is going to be a button, we'll have to be a bit more conscious of what the accessible names look like. By default, it's the text content of the control, which right now would probably be something like "Sample Page admin published", which isn't great. The best way to fix this would probably be by setting aria-labelledby to point at the primaryField content.
That being said, the main job of an accessible name is to convey the purpose or intent of the element, and just using the primary field value doesn't really do that. So we'll have to think a bit about the semantics around this; a straight-up list of buttons might not actually be the best model here anyway.
There was a problem hiding this comment.
Can you explain the benefit of doing this a bit more?
For sighted users the visual noise of the constantly-visible chevron is a bit distracting. It feels overly prominent in the UI. Perhaps a bad example but I'd liken it to tabs in a web/file browser, IDE, etc., where the close button is generally not permanently visible.
If the overlapping button is going to be problematic perhaps we can try a different hover/focus treatment of the main button. I'll try a design iteration tomorrow.
There was a problem hiding this comment.
@andrewhayward it occurred to me that the pattern I'm describing here is virtually identical to List View in the Editor. While not perfect, it has been quite rigorously tested. Do you think that since there's a precedent we might mimic that pattern here?
Edit: To be clear, the visuals still likely need some adjustment, particularly the hover state.
There was a problem hiding this comment.
Here's what I have:
Gravacao.do.ecra.2024-01-08.as.17.55.12.mov
Implemented it following the block editor list view pattern:
- structurally, buttons are siblings
- most styles are handled by the button's wrapper
- the focus styles are attached to a
::afterpseudo-element
There was a problem hiding this comment.
Looking at how the block editor list view works from a keyboard perspective, a follow-up to this PR could be to make dataviews' list view work the same: tab only stops at the top-most item of the list layout, people use arrow keys to navigate through the items and accessing the details.
|
Related PR about making pages-dataviews the default for the "Pages" section: #57589 |
oandregal
force-pushed
the
add/page-details-list-dataviews-link
branch
2 times, most recently
from
ecc4ebd to
a75d5ab
Compare
|
Trying to land this iteratively was creating some weird interim flow experiences, so implemented the main flows here: Gravacao.do.ecra.2024-01-05.as.14.15.46.mov |
oandregal
changed the title
ntsekouras
reviewed
Jan 5, 2024
oandregal
changed the title
youknowriad
reviewed
Jan 5, 2024
packages/edit-site/src/components/sidebar-navigation-screen-main/index.js
Outdated
Show resolved
Hide resolved
oandregal
force-pushed
the
add/page-details-list-dataviews-link
branch
from
8dc94c6 to
1ad99df
Compare
oandregal
commented
Jan 8, 2024
|
@oandregal I just pushed some style tweaks. One thing I couldn't fix was the display of the preview, title, and fields – they're still quite a bit different to trunk:
I'm not fully convinced about the |
oandregal
force-pushed
the
add/page-details-list-dataviews-link
branch
from
3d19480 to
8096b52
Compare
|
This is the current state: Gravacao.do.ecra.2024-01-09.as.09.34.31.mov@jameskoster is there any other design/interaction feedback you want incorporated in this PR? I'm looking now at other feedback and hoping to land this soon to unblock follow-ups. |
oandregal
commented
Jan 9, 2024
oandregal
commented
Jan 9, 2024
| ) } | ||
| </div> | ||
| <HStack> | ||
| <li |
There was a problem hiding this comment.
The changes here are to accommodate the two buttons as siblings. This is what we want:
<ul>
<li>
<div role="button">...</div> <!-- CONTENTS -->
<button /> <!-- DETAILS BUTTON -->
</li>
</ul>Then, in CSS, we make it look like the first button (contents) wraps the second in situations such as focus. See accessibility conversation.
oandregal
commented
Jan 9, 2024
| &:focus { | ||
| box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color); | ||
| &::before { |
There was a problem hiding this comment.
This pseudo-element serves to visualize the focus ring as it was part of the parent node (.dataviews-list-view__wrapper). See conversation.
oandregal
force-pushed
the
add/page-details-list-dataviews-link
branch
from
8096b52 to
537e399
Compare
|
One thing that could use a refinement is the focus styles for the details button when the item is selected (cc @jameskoster): Gravacao.do.ecra.2024-01-09.as.11.06.39.mov |
I just pushed a fix for this. |
oandregal
commented
Jan 9, 2024
| OPERATOR_IN, | ||
| } from '../../utils/constants'; | ||
|
|
||
| export const DEFAULT_CONFIG_PER_VIEW_TYPE = { |
There was a problem hiding this comment.
This was moved from page-pages/index.js to initialize the DEFAULT_VIEWS with the layout data, which was not being initialized. It wasn't an issue until now because the table doesn't have any layout data, but the list does.
jameskoster
approved these changes
Jan 9, 2024
|
I'm merging this to continue work in the follow-ups. Please, do bring up any follow-up I've missed in post-merge reviews, so it's tracked and can be addressed. |
youknowriad
reviewed
Jan 9, 2024
| @@ -236,7 +236,8 @@ Array of operations that can be performed upon each record. Each action is an ob | |||
| - `isLoading`: whether the data is loading. `false` by default. | |||
| - `supportedLayouts`: array of layouts supported. By default, all are: `table`, `grid`, `list`. | |||
| - `deferredRendering`: whether the items should be rendered asynchronously. Useful when there's a field that takes a lot of time (e.g.: previews). `false` by default. | |||
| - `onSelectionChange`: callback that returns the selected items. So far, only the `list` view implements this. | |||
| - `onSelectionChange`: callback that signals the user selected one of more items, and takes them as parameter. So far, only the `list` view implements it. | |||
| - `onDetailsChange`: callback that signals the user triggered the details for one of more items, and takes them as paremeter. So far, only the `list` view implements it. | |||
There was a problem hiding this comment.
I think there's a world where this is the same as onSelectionChange in other words, when a single item is selected in list view we navigate to "details" and vice versa
There was a problem hiding this comment.
That's possible, indeed. The way it works now is optimized for being a listing: you navigate through the items, preview them, and can manage them. There's also the multi-selection use case for bulk editing that needs to be accommodated.
|
Follow-up that adds a "Add new page" button at #57685 |
|
Follow-up that adds a footer to the sidebar with the |
This was referenced Jan 11, 2024
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.
Part of #55083
Supersedes #57523 #57589
What?
Updates the root "Pages" section to go to pages dataviews. It also adds a "details" button to support the whole flow:
Gravacao.do.ecra.2024-01-09.as.09.34.31.mov
Why?
It's the target flow we want.
How?
Makes the
listlayout the default.Adds a "details" button to the list view item.
onDetailChangecallback to the DataViews API, so consumers can handle this action. Note that, at the moment, the details event redirects the user to the existing details page (see conversation). The immediate follow-up is to open this in the content frame, as per the mockup below.Updates the flows, so the back button works as follows: "Pages < DataViews < Details".
Testing Instructions
Test pages show the details:
Test the templates do not have the details button and works as before:
Follow-ups
Pages#57685What'd be the best place? In the sidebar (like the current design) or in the content frame (like we do for templates)? Related conversation.Make it work like the block editor list view: use arrow keys to move up/down the list and right/left to move to/from details.