Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explore Multi-Entity Saving #18029

Merged
merged 15 commits into from Dec 12, 2019
Merged

Explore Multi-Entity Saving #18029

merged 15 commits into from Dec 12, 2019

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Oct 18, 2019

Follows #17368

Description

This PR explores what it could look like to save multiple entities at the same time using a modal with a list of checkboxes that let you select a set of entity changes to save.

How has this been tested?

It was verified that:

  • The new button appears only when there are pending changes to an entity and the full site editing experiment is enabled.
  • Clicking the button opens a modal with a checkbox list of entities with pending changes and their changed properties.
  • Selecting a set of entities and clicking the save button works as expected, only saving the selected entities.
  • If there are no selected entities, the save button is disabled.

Screenshots

Screen Shot 2019-10-18 at 1 23 01 PM

Types of Changes

New Feature: The full site editing experiment now has a button for selecting and saving multiple entities at once.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 21, 2019

Nice work. I don't have time to review in depth today, will test this out tomorrow. However based on looking just at the screenshots, here are some high-level thoughts:

  1. We need to not have a separate "Save global edits" button.
  2. If only ever one thing changed, we should not need a prompt.

Those are principles we should aim for, but obviously there's a lot of technical reasoning to work out. For example, in the screenshot, what is actualy being saved? A draft? Would pressing Save on the blue button be the same as pressing "Save Draft"?

Tricky use case: user is working on a draft and is not ready to publish it. But they also made changes to the Header block area. What happens?

A few thoughts to help us work through it:

  • I still believe that "save draft" is not something a user should ever have to think of. It should happen automatically and there should not be an explicit save button unless something went wrong. This is the Figma auto-save model.
  • If a user is working on a draft and made global edits, perhaps we should hide the "Save Draft" button until the global edits are published.
  • Given global edits are being published, perhaps the actual Publish button should be used.

Screenshot 2019-10-21 at 11 56 04

That's a screenshot from #13489 (comment). In it, global changes were made, so there's not a "Save Draft" button, instead there's a "There are unpublished changes" label. The way you publish those is using the Publish button, which then opens the modal.

But perhaps that label needs a little love. Here's a tweaked flow:

Publishing Block Area Changes

Note in the above:

  • How there's no save draft button.
  • That there's a Publish Changes label.
  • That the Page itself, which is a draft, is unchecked by default.

What would happen if you pressed "Publish" in the above screenshot, is that you would:

  • Publish the header change
  • Get the Save Draft button back, because now you're just editing a draft
  • The Publish button would change to say "Publish".

What do you think?

Also, CC: @shaunandrews because I know you have thoughts here as well.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Oct 21, 2019

We need to not have a separate "Save global edits" button.

Yes, but some sort of separation like this would make things easier to grasp.

If only ever one thing changed, we should not need a prompt.

What if that one thing is not the actual post you are editing. It'd be confusing if you saved as usual.

I still believe that "save draft" is not something a user should ever have to think of. It should happen automatically and there should not be an explicit save button unless something went wrong. This is the Figma auto-save model.

I agree, and this would simplify our problem here. We would no longer have to think about what the save draft button does in relation to global edits.

If a user is working on a draft and made global edits, perhaps we should hide the "Save Draft" button until the global edits are published.

What if users want to save their post content, but not the other changes yet.

Given global edits are being published, perhaps the actual Publish button should be used.

This creates confusion between publishing global changes in a draft and actually publishing the post.

But perhaps that label needs a little love. Here's a tweaked flow:

This is nice, but a bit confusing. What if I want to save the draft without publishing the global changes? I would have to manually undo all the global changes? How does "Publish Changes" convey to users that they're doing something different to "Publish", what if this is an already published post?

I think that the optimal scenario is to have the auto-save draft like Figma for post content, and then for the global changes, we should either style or iconize the publish button to convey that the modal will open, and this can let them just save or publish just the post content, or we can have another button next to the publish button appear, something along the lines of "Publish Templates".

I think the styling approach might be a bit more elegant. What do you think?

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 22, 2019

You make some good points, all worth addressing.

By not adding additional buttons, I mean to the toolbar. There literally isn't space:

Screenshot 2019-10-22 at 10 20 07

If we absolutely have to have a separate button to publish changes made to template parts, we have to explore other designs, such as changing the publish button contextually, or making it a split button or something else. But I don't yet fully accept that this is necessary.

Yes, but some sort of separation like this would make things easier to grasp.

This is a balancing act that has to consider how often you make global edits compared to how often you just write a post or a page. One extra button discussed in isolation may seem like a small thing, but it all adds up.

Especially with something as complex as being able to edit MULTIPLE content areas at once, it is crucial that the flow is simple.

By not having an extra button, but instead bundling multiple changes with the Publish flow, we can keep the writing flow simple, yet give both context, information, and control to the user during the publishing act.

This is nice, but a bit confusing. What if I want to save the draft without publishing the global changes? I would have to manually undo all the global changes? How does "Publish Changes" convey to users that they're doing something different to "Publish", what if this is an already published post?

Maybe you're right — maybe we don't touch the save draft button, and all it does is save the post content. No new label indicating unsaved global changes, no extra buttons. But maybe we still change the label of the Publish button, and then show the popup. We might even show an "unread dot" or something, overlaid on the Publish button. Like this maybe?

Publishing Block Area Changes

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Oct 22, 2019

What is the minimal version of this we can ship today that doesn't impact the non-FSE experience.

For me, it should be something like:

  • you just edit a post UI stays like today
  • enable FSE, UI stays like today if you just edit a post
  • as soon as you edit something global, we can adapt the UI: Show the "Publish Changes"/"Update Changes" like the mockup above and leave the "save draft" button as is. (potentially in a v1, we can remove the "save draft" button entirely from the main UI since all this is still experimental).

This also makes me thing we should hide the "Site Title" block behind the FSE flag for now (the other PR).

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 22, 2019

as soon as you edit something global, we can adapt the UI: Show the "Publish Changes"/"Update Changes" like the mockup above and leave the "save draft" button as is. (potentially in a v1, we can remove the "save draft" button entirely from the main UI since all this is still experimental).

Even just to test this, I would agree this is a good way to start. It let's us learn more about what the pain points are of saving, before optimizing too far in one direction. It's also quite close to this mockup: https://user-images.githubusercontent.com/1204802/67269874-cdc5de00-f4b7-11e9-9b7c-ba73ddf2f6d2.png

@Copons Copons mentioned this pull request Oct 22, 2019
5 of 5 tasks complete
@Copons

This comment has been minimized.

Copy link
Contributor

Copons commented Oct 22, 2019

I was trying to see how many different properties are handled in the Save Global Changes popup, and accidentally stumbled upon the Visibility selector.
Clicking on "Private" triggers a save and (privately) publish after accepting a popup.

Screenshot 2019-10-22 at 15 12 29

In other words, just a warning that the Update/Publish button is not the only one that saves the post. 🙂

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Oct 22, 2019

Maybe you're right — maybe we don't touch the save draft button, and all it does is save the post content. No new label indicating unsaved global changes, no extra buttons. But maybe we still change the label of the Publish button, and then show the popup. We might even show an "unread dot" or something, overlaid on the Publish button. Like this maybe?

Yep, that's what I think is optimal and that is what I meant by styling or iconizing the publish button when there are global changes. Some sort of "plus" or "more" icon comes to mind.

For me, it should be something like:

That's how this PR works. I just think we should change it from rendering a new button to adding an icon to the publish button like discussed above.

This also makes me thing we should hide the "Site Title" block behind the FSE flag for now (the other PR).

Yeah it is behind the flag.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 22, 2019

One thing I just remembered, which is a mistake in the mockups. Any "Publish" action that opens a modal dialog should have the "..." ellipsis at the end. So ● Publish Changes... just like we have Publish.... The ellipsis implies it's the start of a flow.

@shaunandrews

This comment has been minimized.

Copy link

shaunandrews commented Oct 22, 2019

I'm of the opinion that most of this should just work and be handled "behind the scenes" without requiring users to make a decision or understand the entities and how they are related. Maybe I'm oversimplifying all of this, but I keep running through scenarios:

If I update the contents of a page, update the site title, and then I publish my changes they should all be saved and published.

If I update the contents of a page, update the site title, and then save the page as a draft the changes to the site title should also be saved as a draft.

If I update the site title, but never touch the contents of the page, and then publish my changes the site title should be updated.

If I update the site title, but never touch the contents of the page, and then schedule the page to publish, the changes to the site title should be saved and then published at the scheduled date.

--

Regardless of the above, I wonder why we're exploring a modal UI for showing/confirm changes to entities. We already have the pre-publish confirmation, and I think that would be a better place for showing all entities affected by saving/publishing/scheduling. Having it in the pre-publish confirmation would allow us to show the changes, but users could essentially ignore the various entities and just publish using the logic laid out above.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Oct 22, 2019

For a lot of entities it doesn't make sense to draft or schedule. Those scenarios aren't really helpful to users and could create a lot of confusion.

What happens if I schedule different site title updates by scheduling two different posts? I assume the latest one will overwrite the previous one, but that is really confusing. We would need an entire API for this and a UI for warning and surfacing conflicts to users.

Furthermore, it becomes really hard to picture all the different things your save action will be affecting and users could unintentionally edit pages or template parts they didn't mean to.

This is also partly why the modal is decoupled from the pre publish flow, because these entities don't necessarily have a pre publish flow or any flow at all besides saving.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Oct 23, 2019

@jasmussen @youknowriad

One thing I just remembered, which is a mistake in the mockups. Any "Publish" action that opens a modal dialog should have the "..." ellipsis at the end. So ● Publish Changes... just like we have Publish.... The ellipsis implies it's the start of a flow.

I just realized that this doesn't address the potentially very common use case of publishing global changes made while editing a draft you don't want to publish yet. This was the reason I opted for a separate button in the first place. Any ideas? We can always make publishing optional in the modal, but then we muddy the meaning of the button.

I think a separate button could work really well. In addition to the button we could have every block that saves outside the post have a special outline indicator to show that it has pending changes, and on hover we could give the option to save just that block.

The global changes save button would then become a quick way of saving all of these special blocks.

Have you thought about how we could build this as part of the block navigation toolbar. Just like the editor canvas, it could also highlight blocks like these and provide a "save all" button at the top.

Here's a doodle:

Screen Shot 2019-10-22 at 5 37 09 PM

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Oct 23, 2019

I just realized that this doesn't address the potentially very common use case of publishing global changes made while editing a draft you don't want to publish yet. This was the reason I opted for a separate button in the first place. Any ideas?

I still think a single button is the way to go. The user won't have to look into different places to find the save button for the thing he wants to save. I feel some of this complexity can be absorbed in the modal directly, maybe instead of having checkboxes per entity, we could have addition buttons next to each entity where it make sense. (For example a "save draft" button next to the post entity...)

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 23, 2019

I just realized that this doesn't address the potentially very common use case of publishing global changes made while editing a draft you don't want to publish yet. This was the reason I opted for a separate button in the first place. Any ideas?

I tried to address this in the mockup, by way of this bit:

Screenshot 2019-10-23 at 09 03 31

In this one, the About Us page which is in draft form is deliberately unchecked when you press the "Publish Changes" button. Which means when you press the Publish button, the only thing you publish is the change to the Header.

I still think a single button is the way to go. The user won't have to look into different places to find the save button for the thing he wants to save. I feel some of this complexity can be absorbed in the modal directly, maybe instead of having checkboxes per entity, we could have addition buttons next to each entity where it make sense. (For example a "save draft" button next to the post entit

Strongly agree. Not just to keep this PR simple and minimal so we can actually test in master before we optimize down a path we're unsure of, but to keep things simple.

@Copons

This comment has been minimized.

Copy link
Contributor

Copons commented Oct 23, 2019

Regardless of the above, I wonder why we're exploring a modal UI for showing/confirm changes to entities. We already have the pre-publish confirmation, and I think that would be a better place for showing all entities affected by saving/publishing/scheduling. Having it in the pre-publish confirmation would allow us to show the changes, but users could essentially ignore the various entities and just publish using the logic laid out above.

@shaunandrews I think this is a very valid point.
If there are site-wide changes, we could force the pre-publish confirmation which could look something like this (disregard the very awkward copy of course):

Screenshot 2019-10-23 at 11 39 14

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Oct 23, 2019

@jasmussen

Good idea, labelling drafts and having them unchecked by default does the trick.

@Copons

I also think that hooking into the pre publish flow like that is a better idea.

@jasmussen

What do you think about going with that pre publish flow, and adding a button that says, "only publish site-wide changes", to accommodate for people publishing site-wide changes without the drafted post.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Oct 23, 2019

What do you think about going with that pre publish flow, and adding a button that says, "only publish site-wide changes", to accommodate for people publishing site-wide changes without the drafted post.

I think it's a pretty decent idea. I would love to see it explored slightly more in mockup form (@shaunandrews have the energy? I don't think we need to see much).

The challenge is that the pre-publish flow is already pretty heavy. That's not to say it isn't the right place for it, and I still personally have a slight preference for the dialog. But the arguments presented hold promise.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 9, 2019

@aduth thanks for the review! All done 😄

Copy link
Member

aduth left a comment

I seem to be having some trouble with saving, when edits include those to a block and to a separate entity.

  1. Navigate to Posts > Add New
  2. Provide a title to the post
  3. Publish the post
  4. Add a Site Title block
  5. Edit the site title in the Site Title block
  6. Click "Publish..."
  7. Select both options, to save both the post and the site title
  8. Press "Save"
  9. Reload the page
  10. Note that you are not prompted that there are unsaved changes (indicating that the editor considers everything to have been saved), but when the page finishes reloading, the block added in Step 4 is absent.
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Dec 12, 2019

I seem to be having some trouble with saving, when edits include those to a block and to a separate entity.

There was a race condition with the modal setting isSaving to true and then making the held callback's dispatched savePost bail early. I fixed it.

savePost makes sure the latest serialized block content is in core-data while the modal's save hook doesn't, which is why it missed the most recent block.

I also implemented all other suggestions and wrote tests for the new selectors.

@epiqueras epiqueras requested a review from aduth Dec 12, 2019
@epiqueras epiqueras force-pushed the try/multiple-entities-saving branch from 7200301 to acbf0a3 Dec 12, 2019
@epiqueras epiqueras merged commit cc44f8b into master Dec 12, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras epiqueras added this to Done in Phase 2 via automation Dec 12, 2019
@epiqueras epiqueras deleted the try/multiple-entities-saving branch Dec 12, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 13, 2019

Thanks everyone for the reviews and work here.

I guess the next step here would be to consolidate these pre/post publish flows. It would be good to have some design direction/exploration/mockups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.