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

Save all blocks on page save #328

Closed
3 tasks
ScopeyNZ opened this issue Aug 16, 2018 · 8 comments
Closed
3 tasks

Save all blocks on page save #328

ScopeyNZ opened this issue Aug 16, 2018 · 8 comments

Comments

@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Aug 16, 2018

In-line edited blocks can be saved individual but should be saved (en masse) when a page save is triggered. This would involve either looping through all "dirty" blocks and triggering a save (asynchronously) or creating some form of aggregation tool for multiple FormBuilderLoader forms.

I'm not entirely sure on the best approach here.

A/Cs

  • Saving a page with an elemental area will also save all edits to blocks
  • Blocks that have been edited but collapsed without saving are still saved
  • Other page elements save as expected

Split from #295

@robbieaverill
Copy link
Contributor

So (I think) you'd only ever have one block's edit form open at at time.

Is the intended behaviour that when you open block A, change stuff then open block B that the changes to block A should be saved automatically, or stored in the browser in a draft state? If it's the former then this ticket should just be "save the active block on page save" since there'd only be one. If it's the latter then we'll need to be careful that the FormBuilderLoader doesn't overwrite any changes you've made when you reopen the block's edit form again.

@ScopeyNZ
Copy link
Contributor Author

I discussed this with @clarkepaul. You can have multiple in-line edit forms.

@brynwhyman
Copy link

We need to work out what should happen when an invalid block can't saved alongside a block that can be saved. Valid blocks and page saves, while the invalid block has an error message?

For now, this issue is being updated to develop a POC that attempts asynchronous saving that prevents the full page and containing blocks from being saved if there is an invalid block that can't be saved.

cc @clarkepaul @chillu

@brynwhyman brynwhyman added this to the Sprint 21 milestone Aug 31, 2018
@chillu
Copy link
Member

chillu commented Sep 2, 2018

For now, this issue is being updated to develop a POC that attempts asynchronous saving that prevents the full page and containing blocks from being saved if there is an invalid block that can't be saved.

I think a page-level save operation should be atomic: Either it saves, or it doesn't (and returns with errors). That should apply to all blocks as well (implemented via database transactions). What's the purpose of "incremental" saving of valid blocks, while not saving invalid blocks? Do you expect the author to abandon the page edit without rectifying any validation errors and attempting another save?

Regardless of validation errors or not, unsaved changes should prompt a warning when navigating away from an edit form, so we need to ensure this is the case after a page-level save with validation errors in blocks.

Implementation note: Please don't fire off one XHR request per block for this, it'll cause performance issues.

@ScopeyNZ
Copy link
Contributor Author

ScopeyNZ commented Sep 3, 2018

I've created #367 to actually come up with a solution to this problem. This should be blocked until we have a good direction to take this.

I've removed the A/C:

  • Develop a POC that attempts asynchronous saving that prevents the full page and containing blocks from being saved if there is an invalid block that can't be saved.

This is really a solution disguised as an A/C anyway - and it's not clear that this would actually be useful work.

/cc FYI @chillu

@NightJar NightJar removed this from the Sprint 21 milestone Sep 3, 2018
@chillu
Copy link
Member

chillu commented Sep 5, 2018

I think a page-level save operation should be atomic

Note that I've put this stance a bit more into context over at #367 (comment). In short, performance is king, everything else is flexible - but needs careful evaluation of options 🚀

Please don't fire off one XHR request per block for this, it'll cause performance issues.

Same here, this refers to "for every block". It might be feasible if we only do it for "dirty" blocks.

@clarkepaul
Copy link

clarkepaul commented Sep 10, 2018

I was initially hoping we can save what can be saved if it is a self contained thing (eg. content block), leaving only the invalid block/page(s) unsaved, isn't that how we deal with publishing multiple pages (or bulk publishing/campaigns), some might fail but others will publish?

After some thought, parts of a page might be dependant on other parts being there (eg. links to other areas), and require they go live at the same time. Based on this scenario it makes sense that everything be published at the same time, as long as the changes aren't lost it'll be all good.

@robbieaverill
Copy link
Contributor

Done in #439

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants