Pages: update useView logic
#63889
Merged
Pages: update useView logic
#63889
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. |
oandregal
added
[Type] Bug
oandregal
commented
Jul 24, 2024
|
Size Change: -57 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
oandregal
changed the title
youknowriad
reviewed
Jul 24, 2024
oandregal
force-pushed
the
update/active-view-logic
branch
from
4bf53ef to
9dfc39d
Compare
|
I've updated this PR based on this slack thread. In addition to prevent view config resets when the layout changes, this PR also changes user behavior: when selecting one of the default views, the layout is reset to the preferred by the default view. This change will need to be done in Templates & Patterns as well. I haven't yet tested the custom views, but wanted to share what I have for thoughts. |
oandregal
commented
Jul 30, 2024
| }, | ||
| ], | ||
| }, | ||
| return [ |
There was a problem hiding this comment.
The main change here is removing the [ postType ] key.
oandregal
commented
Jul 30, 2024
| icon: trash, | ||
| view: { | ||
| ...DEFAULT_POST_BASE, | ||
| type: LAYOUT_TABLE, |
There was a problem hiding this comment.
I also added type and layout to this default view, to account for switching the trash view to the table layout. This was previously done with the useSwitchToTableOnTrash hook.
oandregal
commented
Jul 30, 2024
youknowriad
reviewed
Jul 30, 2024
| const { params } = history.getLocationWithParams(); | ||
| history.push( { | ||
| ...params, | ||
| layout: defaultView.type, |
There was a problem hiding this comment.
For me ideally, this should not be necessary. I think the "layout" argument in the url should be removed from the links instead.
There was a problem hiding this comment.
mmm, I'm not sure I follow: don't we need to make sure the layout reflects the view type? Or are you saying that you don't want the layout URL when the view type is list?
Note we need to update the layout when a new default/custom is loaded. This was a source of complexity/bugs: in trunk, the trash view has its own custom hook for this and custom views that use a layout different than list don't load properly.
There was a problem hiding this comment.
I pushed 6958d22 to make it work like trunk.
If there's no layout URL param (first time the Pages page loads) and the view.type is list, the URL param won't be updated. Note that, like trunk, once the layout param is present (user changes layout in dataviews, user changes to a default/custom view with a view.type different than list, etc.), it'll stay there even if the user changes back to list.
oandregal
force-pushed
the
update/active-view-logic
branch
from
9dfc39d to
d46406f
Compare
oandregal
commented
Aug 1, 2024
| * When going out of the trash view, it switches back to the previous layout if | ||
| * there was an automatic switch to table layout. | ||
| */ | ||
| function useSwitchToTableOnTrash() { |
There was a problem hiding this comment.
This is removed, we don't want specific logic to any default/custom view. They all should work the same, see slack thread in core-editor channel.
|
Both default & custom views work now with latest code changes. This is how it works:
To cover all use cases, I essentially rewrote the |
|
Marked some old conversations as resolved because the implementation changed significantly. |
oandregal
force-pushed
the
update/active-view-logic
branch
from
e7c1564 to
7047e49
Compare
oandregal
changed the title
useView logic
| } | ||
|
|
||
| if ( newView ) { | ||
| setViewWithUrlUpdate( newView ); |
There was a problem hiding this comment.
I think when we switch to a custom view, as here we call setViewWithUrlUpdate which has this code if ( isCustom === 'true' && editedEntityRecord?.id ) { editEntityRecord(, we will endup calling get editEntityRecord just because of loading the view which may trigger a rerender. Not a big issue.
There was a problem hiding this comment.
That's right. I've looked at this but don't see a way to improve it at the moment. Custom views are an experimental feature and this isn't a big issue, so I'll go ahead with this PR anyway because it provides a lot of value.
I'd be happy to pair/look at this in a follow-up.
jorgefilipecosta
approved these changes
Aug 1, 2024
oandregal
force-pushed
the
update/active-view-logic
branch
from
7047e49 to
272d355
Compare
|
Pushed changes to address all feedback and fix the e2e tests. I've set this to auto-merge. |
oandregal
commented
Aug 2, 2024
| @@ -438,9 +438,7 @@ test.describe( 'Site Editor Performance', () => { | |||
| await Promise.all( | |||
| Array.from( { length: perPage }, async ( el, index ) => { | |||
| return await page | |||
| .getByRole( 'link', { | |||
There was a problem hiding this comment.
This test assumed the page was using the table layout, which was the active one when the Trash view was loaded — and remained so. This PR changes that behavior and always loads the layout associated with the default view, hence at this point the layout will be list.
The test failed because the Pages are links in the table layout but they are not in the list layout.
|
There was an issue with this PR that I'm fixing at #64306 |
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.
Alternative to #63854
Raised at #63203 (comment)
What?
This PR updates the
useViewhook to do the following:layout-changes-do-not-alter-view-config.mov
views-reset-previous-view-config.mov
list, no matter the custom view layout.load-custom-view-layout.mov
http://localhost:8888/wp-admin/site-editor.php?postType=page&activeView=wrong-keywrong-key.mov
Testing Instructions
Verify those work as expected and test default/custom views in general.