Editor: Multi-Entity Saving UI Improvements #35933
Conversation
ockham
added
[Feature] Synced Patterns
|
Size Change: +4.58 kB (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
Testing method: https://make.wordpress.org/design/2021/03/03/testing-a-gutenberg-pull-request-pr/ This is what I see. (I happen to test out a General template called Test.)
Here are some thoughts that show up as I go through these sections. Are you ready to save? (Kinda strange seeing that here as I have already clicked the Save button once. Next step is to go through the various areas listed. Make needed adjustments unclick (or some other way) to discard what I do not want to save and then click Save again to save the areas I want saved.) "The following changes have been made to your site, templates, and content." Site - "These changes will affect your whole site." (To the point and helpful.) Template - "These changes will affect pages and posts that use these templates." (To the point and helpful.) Template Part (should we have a sentence here?) Something like: These changes will affect the following template parts. As in a real life situation header might be called header front page, header contact page, footer front page etc. Page - Home
affects the Page - Home. Similar to how further up the Template panel has this sentence: "These changes will affect pages and posts that use these templates." Thanks. |
|
Thanks for reviewing, @paaljoachim! I've mostly tried to stick to the wording specified by the designs in #31456 (comment) (with one minor change, I put "The following changes have been made to your site, templates, and content." rather than "The following changes have been made to your site, post, and templates." to reflect that the changes can affect any kind of content -- rather than just a post -- and to reflect the new entity order). That said, I don't mind changing the copy; but I would like to check back with designers, as per my understanding, we need to strike a balance between providing enough explanatory texts to help the user understand what they're doing, but not too much as to not overwhelm them 🙂 |
ockham
force-pushed
the
update/multi-entity-saving
branch
from
3316847 to
3e0e9cd
Compare
|
Investigating failing e2e tests 👀 |
Pushed a fix: 034eace |
|
This works great, @ockham! I have just some minor suggestions for improvements:
And here's something I just noticed when I saved a post after changing its title: we are currently showing the new title. Could we say "Title" as we do with the site?
+1, much better than what I suggested. |
|
Hey @javierarce, thanks a lot for reviewing! I'll address each item in a separate comment, starting with one of the easiest ones:
|
|
That's not really specific to title changes, though: It will refer to the modified post by title irrespective of what part of the post was changed. (Try e.g. changing the post date through the corresponding block.) So I'd rather not change that 🤔 |
Oh, I didn't notice that, sorry. Ok, let's leave it then. |
|
Javier I believe you mentioned merging templates and template parts into one panel + capitalizing Header and Footer. Like so:
NB! I added some text under the Pages panel. |
This one was a bit trickier than it seems. The template part names are specified by the template part itself, and could theoretically be pretty much any string -- and in some cases maybe shouldn't be capitalized (think i18n). What we seem to do in cases like this is find the template part area a template part is meant for. For some of those (such as header and footer), we've defined specific icons and labels (which are properly capitalized, and -- hopefully! -- translated). I've implemented this in 4ef8ac1:
|
Hmm, this is fine, though: The text is really only meant to refer to templates. The Page panel however is about the page content (rather than the template) that the user has modified.
We can do that, but it's a bit misleading, and it won't scale too well: It's technically correct since AFAICT, we'll only see this panel if we're editing the template for a specific page (rather than a generic template, such as Single Post or Index). However, as stated above, the other necessary condition for seeing this panel is that we've modified the page content. So it would be more correct to say something like "The content of the following page has been modified." However, if we allow editing content other than individual pages in the site editor -- e.g. multiple posts through a template with a query loop -- that description would no longer be accurate. How about something like "The following content has been modified." instead? |
|
This sounds good to me. What do you think @javierarce ?
I assume we should have @kellychoffman and @jameskoster check out the PR as well. |
This item is the trickiest one. I'm not totally sure I agree that having templates and template under the same heading simplifies thing 😬 On the development side, it would actually be a rather big change of the relevant code. I'd just like to double-check this is really what we want before I make that change 😅 From a UX-perspective, it was my impression that we have a few invariants that differentiate both concepts:
|
After thinking this through again and reading your explanation, I feel that it could be confusing to list the name of a template (say 404) and then "header" and "footer" immediately below. For me, templates and template parts are conceptually very similar (they are structures), but the difference in size and side-effects at that part of the flow could be disorienting for a user who has made many changes. The solution that @paaljoachim designed in this comment sits in a good middle ground that could simplify the sidebar and at the same time maintain that differentiation. The only thing I would change is the "Template part" label: I would use "Areas" as we do in the header dropdown. |
There was a problem hiding this comment.
Thanks for working on this @ockham!
This is looking good code wise and my sole concern is the template parts area comment.
packages/editor/src/components/entities-saved-states/entity-type-list.js
Outdated
Show resolved
Hide resolved
packages/editor/src/components/entities-saved-states/entity-record-item.js
Outdated
Show resolved
Hide resolved
This reverts commit 4ef8ac1.
Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com>
| ), | ||
| }; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
I guess this can become a bit overwhelming with many changes 😄
There was a problem hiding this comment.
That was partly by mistake -- there shouldn't be any description for Template Part. Fix in 717d7041ba.
I don't mind removing the "The following content has been modified." description altogether, if it's still too overwhelming.
There was a problem hiding this comment.
I personally like it for the page edits. Not sure for reusable blocks though, but not a blocker.
There was a problem hiding this comment.
Ah, I'd somehow missed the reusable blocks. Good point, I'll do some final tweaking...
Thanks @javierarce! I believe that the latest couple of commits should've tweaked the UI accordingly (see image at the bottom of this comment).
In line with the outcome of my discussion with @ntsekouras and @Mamaduka, I've kept this as "Template part", since we really show the template parts that are included with a theme, and also use their titles (rather than the areas they're associated with) -- which could be something like "Default header", "Alternative header", "Big footer", "Small footer" etc, since a given theme can theoretically provide multiple template parts for a given area. What I've done however is pluralize correctly if there are multiple template parts affected:
|
|
Thanks for working on this, @ockham. |
There was a problem hiding this comment.
The design looks good to me! Thank you for working on this PR @ockham Bernie!
Here is a before and after.
|
Thanks for working on this, @ockham. The changes look good! |
|
Thanks all for your feedback and reviews! ❤️ |
| ); | ||
| case 'page': | ||
| case 'post': | ||
| return __( 'The following content has been modified.' ); |
There was a problem hiding this comment.
Should we move these descriptions to the entities config instead?
There was a problem hiding this comment.
You mean https://github.com/WordPress/gutenberg/blob/ba5f6d061934625027beab9d2bf600ae42a63be9/packages/core-data/src/entities.js? Yeah, that might make sense 👍
| const { | ||
| site: siteSavables, | ||
| wp_template: templateSavables, | ||
| wp_template_part: templatePartSavables, |
There was a problem hiding this comment.
Do you think that order can be defined as a "priority" somehow in the entities config? Or maybe this is sufficient for now?
There was a problem hiding this comment.
I think it'd make the code quite a bit more complex, and I don't think it's worth it right now, since this is the only place so far where we're needing that priority order.
Description
Closes part of #31456.
How has this been tested?
Screenshots
Follow-up
I'd like to add the second screen from #31456 (comment) (where users can discard changes they've made) in a follow-up PR.