Site Editor: update index page for templates #59792
Conversation
|
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] Enhancement
oandregal
changed the title
|
Size Change: +106 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
oandregal
added
the
[Feature] Site Editor
oandregal
changed the title
|
The default views in the sidebar are broken in |
|
Neat! I wonder if Grid should be the default layout? My only concern there is the lacklustre performance... a loading indicator (spinner or progressbar) might help. |
I had the same question and thoughts :) If we want to make it the grid and having a loading indicator is a requirement, I may suggest doing it in a follow-up. I'd like to have all the index pages migrated as soon as possible, so we have all the 6.6 cycle to iterate on these details. |
|
Imo it seems fine to proceed with table as the default layout, then explore making grid the default (with loading indicators) in a follow-up. |
|
Testing this a bit, I think either "grid" or "list" are better default layouts for templates. Table seems better suited for advanced bulk editing. |
|
There's a bug in "list" layout, it might not be specific to this PR though. Select the list view then click "All templates" in the sidebar, the preview disappears. |
This is fixed by #59794 |
oandregal
force-pushed
the
update/templates-index-dataviews
branch
from
07a3cce to
a75e78d
Compare
|
There is an issue when we are in a template details view and click 'back' in the navigation sidebar. The URL should not preserve the Additionally we should update the template actions(namely the |
No changes in this behavior for this PR, it works as in
Ah, nice one — the dynamic paths obscured my search. They are now fixed, thanks! |
oandregal
force-pushed
the
update/templates-index-dataviews
branch
from
dc79498 to
811a492
Compare
|
Thanks for taking the time, Ramon! It seems that there's still an issue, I'll keep investigating. |
oandregal
force-pushed
the
update/templates-index-dataviews
branch
from
9aa8fc9 to
3db3884
Compare
Thank you! So strange... |
|
Agree grid would be a nice default for templates |
| @@ -193,7 +193,7 @@ test.describe( 'Site Editor Performance', () => { | |||
| await metrics.startTracing(); | |||
|
|
|||
| await page | |||
| .getByRole( 'button', { name: 'Single Posts' } ) | |||
| .getByRole( 'link', { name: 'Single Posts' } ) | |||
There was a problem hiding this comment.
I think this is probably the change that is breaking the performance tests in this PR.
If that change is required, we need to keep both "button" and "link" because performance tests compare two branches and old ones would have a "button" while this one will require "link". So we need something that seems button or link here.
There was a problem hiding this comment.
Oh, thanks!
What is this test tracking? The time it takes an editor to load since an item is clicked?
Could I use a different path (loading pages or parts instead of templates)? If I understand this correctly, I see a couple of potential issues with that approach:
- it will change the loading profile for the test, so it can render different results just because we are loading different things than before
- it'll work for this PR because it compares with
trunk, however the issue will happen again when it is compared against the base commit (during the plugin release, I believe).
So, my options boil down to use some kind of flag/check to run one code path or the other in the test (I'm not sure that's even possible?) or find a common locator that works for these two screens:
| Trunk | This PR |
|---|---|
 |
 |
Is this correct?
Going to start looking for the locator. I'd welcome looking at other cases where we ran into the same issue to get inspiration from.
There was a problem hiding this comment.
getByText works locally, let's what CI thinks of it.
There was a problem hiding this comment.
it will change the loading profile for the test, so it can render different results just because we are loading different things than before
Yes, it compares the time it takes to switch between two templates while the preview is visible. I realize that this is changing. So for legacy "template screens" it should continue to work as is, for new ones it's probably better to check in the "list" data view right?
I mean we can change it to pages or others if needed but it wouldn't solve the problem, it will just delay solving it. (when the dataviews get lifted up for these)
There was a problem hiding this comment.
So in other words, we should add a check to the test and if we're in an old branch keep using the regular root /template but if we're in a new branch, we should switch to list view first before navigating. Otherwise comparing old vs new wouldn't make sense.
There was a problem hiding this comment.
Navigate is 5x slower, according to the test. Codehealth reports it to be within 20-35ms, historically. 🤔
There was a problem hiding this comment.
It is interesting indeed. It is possible that the new value is more representative though. It's hard to tell. Maybe with list views, things are more "synchronous". I'm not sure it should block this PR but we should try to understand.
There was a problem hiding this comment.
I've reviewed how the new test works (npm run test:performance:debug -- site-editor -g "Navigating") and it seems to do so as expected.
I'll need to look more into how metric utils for e2e test work to understand what we were measuring before and after — and if we are actually measuring the same thing.
Looking into devtools > performance for how this branch works vs trunk I don't see anything obvious (attach the traces for this branch and trunk, devtools-performance-traces.zip). The click event triggers a ~475ms execution in both this branch and trunk (though with some differences):
| Branch | Trunk |
|---|---|
 |
 |
My suggestion is merging this PR and investigate this independently.
There was a problem hiding this comment.
The screenshot you shared is actually very telling :)
You're right that the overall time is similar but the "click" event which is what we're measuring is way bigger in this PR. In other words, something that was async (like in a micro task or delayed using a promise or something) is becoming sync in this PR.
I don't necessarily think that's a bad thing on its own but it shows that we need to update our performance test to compute the whole stack (clicking and re-rendering) regardless of whether the rendering is async (for some reason).
So I agree that we should merge and try to improve the metric separately.
There was a problem hiding this comment.
So, I've looked at how/what we measure. getClickEventDurations ends up calling getEventDurations filtering by click event type. If I understand how this is implemented, it means we were/are measuring the duration of any click event during the tracing (and not the whole execution or related tasks). In the traces I've shared, the click event is 210ms in this branch vs 57ms in trunk: a ~x4 performance impact, more in line with the numbers reported by our test.
…e SinglePosts item
oandregal
force-pushed
the
update/templates-index-dataviews
branch
from
01606a7 to
0d7b67e
Compare
|
Follow-up to make |
Co-authored-by: oandregal <oandregal@git.wordpress.org> Co-authored-by: mcsf <mcsf@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: SaxonF <saxonafletcher@git.wordpress.org>
Part of #55083
Related #59659
What?
Updates the index view for Templates.
Gravacao.do.ecra.2024-03-12.as.16.07.25.mov
Why?
See #59659
How?
Testing Instructions
Test the flows so the new index view is always as corresponds.