Fix: DataViews: Active page is not highlighted properly in list view. #62378
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
the
[Type] Bug
|
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: +144 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Should this be backported? If so, please add the |
youknowriad
reviewed
Jun 7, 2024
| const { | ||
| params: { postId }, | ||
| } = useLocation(); | ||
| const [ postIdToSelect, setPostIdToSelect ] = useState(); |
There was a problem hiding this comment.
Can we use postId as default value here to avoid initial re-rendering?
There was a problem hiding this comment.
Yes, good catch the code was updated 👍
youknowriad
reviewed
Jun 7, 2024
packages/dataviews/src/dataviews.tsx
Outdated
| onSelectionChange?: ( items: Item[] ) => void; | ||
| } | ||
|
|
||
| const defaultGetItemId = ( item: AnyItem ) => item.id; | ||
| const defaultGetItemId = ( item: AnyItem ) => item.id + ''; |
There was a problem hiding this comment.
Yes it is needed, it is making sure the ID returned is a string, in some cases, it was an integer that made the comparisons fail. The selections have a type of string[] and previously many times it was an integer array.
There was a problem hiding this comment.
our types, say it's always a string. So if we were actually using typescript in the consuming side we'd see the error. So maybe instead of changing the default getting we could provide ( item: AnyItem ) => item.id.toString() as a prop for the case where the id is integer no?
There was a problem hiding this comment.
Hi @youknowriad I applied your suggestion.
jorgefilipecosta
added
the
Backport to WP 6.7 Beta/RC
|
Flaky tests detected in 1d9d2d1. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9418340439
|
youknowriad
reviewed
Jun 7, 2024
| @@ -266,6 +278,20 @@ export default function PagePages() { | |||
| totalPages, | |||
| } = useEntityRecords( 'postType', postType, queryArgs ); | |||
|
|
|||
| useEffect( () => { | |||
There was a problem hiding this comment.
Isn't this a duplicate of the effect above? Also the code feels a bit fragile to me to be honest. Do we expect all dataviews to have this same logic. It's not really straightforward. Is there a way to improve this?
There was a problem hiding this comment.
It is not the same effect as above. Above, when the page gets a postId in the URL parameter, we set that ID as something to be part of the selection. We cannot select it immediately because the items may still be loading.
In this effect, if things are loaded and an item should be selected, we update the selection.
It looks fragile but seems to work well. Previously, I tried other ideas, but they ended up not working all the time or being even more complex. I am not sure how we can simplify this further. What I can do is put the effects and the state inside a hook that we can reuse in other places if needed (and that way, at least the logic is joined in a specific place).
There was a problem hiding this comment.
I made this change to extract the logic in a specific hook.
jorgefilipecosta
force-pushed
the
fix/DataViews-Active-page-is-not-highlighted-properly-in-list-view
branch
from
1d9d2d1 to
a28b020
Compare
|
Not a code review To test this issue, make sure that you are not testing the background color change which happens when you manually click on a page in the list: You need to test the currently active page, so you need to refresh the browser. Before: The list has no visible indicator that a page is selected. 
While there is a light visible background color change, I am unable to detect any indicator that a page is the active page when I am using a screen reader. Is there an open issue for this problem, or are there specific testing instructions for screen readers? The background color alone is not enough to indicate that the page is the active page, the color has too low contrast compared to the default color. |
|
I did a quick test and I am seeing the same issue for the templates (missing indicator of the active template when the list layout is used), and this is not solved by this PR. Is there a separate issue for that? |
oandregal
approved these changes
Jun 19, 2024
|
Thank you for the reviews @carolinan and @oandregal. I will follow up with something similar for the templates.
We are showing aria-pressed="true" do you think we should include other additional aria information? If so I'm happy to iterate on this. |
jorgefilipecosta
deleted the
fix/DataViews-Active-page-is-not-highlighted-properly-in-list-view
branch
The element that includes the information about the page in the list layout is not a button and does not have |
ellatrix
added a commit
that referenced
this pull request
Jun 25, 2024
…#62378) Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: oandregal <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: carolinan <[email protected]>
|
I just cherry-picked this PR to the wp/6.6-rc-1 branch to get it included in the next release: e3097e2 |
ellatrix
added
Backported to WP Core
ellatrix
added a commit
that referenced
this pull request
Jun 25, 2024
…#62378) Co-authored-by: jorgefilipecosta <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: oandregal <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: carolinan <[email protected]>
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.
Fixes: #60708.
This PR adds a "controlled" mode for the selection in the DataViews component (passing the selection as a prop) as suggested by @youknowriad.
Adds logic to make the current link in the URL the selection. For this to work we needed some logic to avoid race conditions e.g.: we should only include the selection if the items are loaded otherwise dataviews code automatically removes the selection.
We also had some cases where the selection id were integers instead of strings and we missed a usage getItemId.
Testing Instructions
Go to http://localhost:7888/site-wp-dev/wp-admin/site-editor.php?postType=page&postId=1246 (1246 needs to be a valid page id)
Verify the active page is highlighted properly.