Add templates list page for site editor #36379
Conversation
kevin940726
added
Backport to WP Beta/RC
kevin940726
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
kevin940726
force-pushed
the
add/edit-site-list
branch
from
b8af001
to
ea3d5b7
Compare
|
Size Change: -2.54 kB (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
|
This is cool to see :) I know it's incomplete, but here's some initial thoughts: When you navigate away from the initial editing view in this environment, it's not clear how to get back. Clicking Styles will get you there, but that's not obvious. I've added an "Editor" link as a placeholder, but this needs some refinement (The "Editor" title followed immediately by an "Editor" link is a bit 🥴). I couldn't get the active state working either.
Happy to help with this :)
This feels especially bad right now, mostly due to the flash of wp-admin you see between page loads which is very disorienting.
What did you have in mind here? I'm not sure we need them. Same goes for pagination – it seems unlikely that sites will contain so many templates we need to paginate – at least not initially. |
Changed "Editor" to "Site" which feels like a step in the right direction, and speaks to the "Site Editor" terminology we've all gotten accustomed to :) Easily changed of course. |
| * @return bool True for Site Editor pages, false otherwise. | ||
| */ | ||
| function gutenberg_is_edit_site_list_page() { | ||
| return isset( $_GET['postType'] ) && ! isset( $_GET['postId'] ); |
There was a problem hiding this comment.
Could you not use get_screen or similar.
There was a problem hiding this comment.
I'm not sure what you mean here? Sorry I'm not really familiar with PHP 😅 .
There was a problem hiding this comment.
I don't think we can. These are query arguments on the Site Editor page.
packages/edit-site/src/components/navigation-sidebar/navigation-panel/index.js
Outdated
Show resolved
Hide resolved
Agreed. I pushed some changes to preload API requests and prevent the jump of the UI. Hopefully it's a lot smoother now, but still not near to be fast. We can explore SPA approach in another PR.
Yeah, I'm not sure we need them either, especially for pagination. I wonder what others think of previews though? |
|
@jameskoster What do you have in mind for the "Actions" menu in the list for each item? What do we want to have in there? Additionally, should only the name of the item be a link, or should we make the whole row clickable? |
tellthemachines
added this to 👀 Needs review
in WordPress 5.9 Must-Haves
via automation
It's feeling much better now :) For the actions:
In the future, all templates should be duplicatable to create a new general template, but I don't think we need that right away. |
|
I've refactored the markup so that this view uses a list rather than a table, which made styling for mobile a bit simpler. One thing we need to address is how the nav functions on mobile because this isn't great:
It might be better for the nav to:
|
|
One simple solution for the nav may be to set the content area min width to 100% on smaller screens. That way it gets "pushed over" on smaller screens, and collapsed on larger screens, like so: nav.mp4A related thought – the admin bar appearing on smaller screens is kind of disruptive, we should probably hide that, just as we hide the admin bar on larger screens. |
I'm afraid that it won't be accessible this way? I could be wrong though. What's specifically been made simpler if it's a list? We could however add those aria roles back though. |
|
Which aria roles? 🤔 Lists are just way more flexible, and semantically seem appropriate for this use case. |
The name and the theme are no longer mapped to the header for instance. There's only a visual clue now but they aren't semantically connected. I'd defer to a11y experts for this though 😅 . |
Ah, yeah I'm not 100% sure on this, but it seems easily fixable if necessary.
Considering how uncommon this is likely to be, I don't think we need to elaborate on it for this PR. Here's how things look currently:
|
I pushed a change that sets the nav to open on desktop, and closed on mobile. I don't know if it's possible to do this on window resize, if so that would be awesome, but otherwise this seems like an adequate fallback. |
There was a problem hiding this comment.
Really impressive work here @kevin940726! Let's merge this and work on the follow-up tasks during coming week. We'll need the important items merged by November 26.
| ), | ||
| } ) } | ||
| href={ addQueryArgs( '', { | ||
| page: 'gutenberg-edit-site', |
There was a problem hiding this comment.
This JavaScript code will end up in Core where the site editor will be at site-editor.php, not themes.php?page=gutenberg-edit-site. This code needs to be updated to add postType to the current location.href.
| href={ addQueryArgs( '', { | ||
| page: 'gutenberg-edit-site', | ||
| // TODO: We should update this to filter by template part's areas as well. | ||
| postType: template.type, |
There was a problem hiding this comment.
It's typical in WordPress that URL query params (and PHP variables more widely) use snake_case, not camelCase.
There was a problem hiding this comment.
It has already been using camelCase before even having the focus mode, I'm not sure why it's the case though.
There was a problem hiding this comment.
Oh I see. Let's leave it then.
noisysocks
removed
the
Backport to WP Beta/RC
* Add edit site list page * Add Editor navigation item * Editor -> Site * Remove style: closed * Preload some API requests * Default to is-fullscreen-mode * style and markup refactor * Set a min width on the content area for smaller screens * Always hide the admin bar * Set the nav to closed on small screens * formatting * Add action to remove template from the list * Rebase and fix conflicts * Fix styles typo Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
| wp_enqueue_style( 'wp-edit-site' ); | ||
| wp_enqueue_media(); | ||
|
|
||
| $template_type = $_GET['postType']; |
There was a problem hiding this comment.
We need to bail here if an invalid postType is provided, otherwise a malicious bit of HTML can be output when it is interpolated into the wp.editSite.initializeList below.
Fix: #36706
|
The site editor in WP trunk seem to be failing for me right now, I think it's because the backend is using |
|
@youknowriad: Have you updated Core trunk? This was updated recently. |
|
@noisysocks for me the backend was correct (initializeEditor) but not the frontend (still using initialize). Maybe it was just a testing issue 🤷♂️ |
Description
Implemented part of the design in #29630 (comment). Close #35994. Close #36352.
This is a big change as it implements a new screen. Though there are still lots of TODOs I intentionally left out for the sake of easier reviews and so that we can divide and conquer.
If you want to work on any of the above, feel free to open another PR, or directly push changes to this PR.
How has this been tested?
tt1-blocksthemeScreenshots
Kapture.2021-11-11.at.01.29.25.mp4
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).