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

NEW All unsaved blocks on a page will now save as the page is saved #439

Conversation

ScopeyNZ
Copy link
Contributor

@ScopeyNZ ScopeyNZ commented Oct 11, 2018

Fixes #328 . This works by syncing redux state with a hidden field. The content of the hidden field is then used to write back into the various elements within an elemental area.

Relies on silverstripe/silverstripe-admin#682

Todo:

@ScopeyNZ
Copy link
Contributor Author

Someone might be able to provide a good solution to optimistic cache updates. If I can get the CMSMain::save response to return some custom data I'm pretty confident I can do it, but it doesn't seem extensible enough. Open to ideas.

public function setSubmittedValue($value, $data = null)
{
// Content comes through as a JSON encoded list through a hidden field.
$this->setValue(Convert::json2array($value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to return $this for chaining, and also these methods could use doc blocks


foreach ($elementData as $form => $data) {
// Extract the ID
$elementId = (int) substr($form, 12);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic numbers!

@ScopeyNZ ScopeyNZ force-pushed the pulls/4.0/save-all-the-things branch 2 times, most recently from e33f5c4 to 95e6ceb Compare October 14, 2018 09:15
@ScopeyNZ
Copy link
Contributor Author

ScopeyNZ commented Oct 14, 2018

@ScopeyNZ ScopeyNZ changed the title WIP NEW All unsaved blocks on a page will now save as the page is saved NEW All unsaved blocks on a page will now save as the page is saved Oct 14, 2018
{
$label = $block->findAll('xpath', sprintf('//label[contains(text(), \'%s\')]', $name));

assertNotNull($label, sprintf('Could not find a label for a field with the content "%s"', $name));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of Title I get the following issue with this:
Found more than one label containing the phrase "Title". Failed asserting that actual size 0 matches expected size 1.

The HTML looks as follows:
<label for="Form_ElementForm_11_PageElements_11_Title" class="form__field-label ">Title (displayed if checked)</label>
and in the feature file I've got
And I fill in "Charlie's Block" for "Title" for block 2

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I found the mistake I made. This works as expected, but the message is wrong. Should assert the count instead.


/**
* {@inheritdoc}
*/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as not having a doc block at all, you may as well leave it out and save the lines

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be good to return $this for chaining, and also these methods could use doc blocks

😆 I'll remove them again 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love me a transparent doc block

public function setSubmittedValue($value, $data = null)
{
// Content comes through as a JSON encoded list through a hidden field.
return $this->setValue(Convert::json2array($value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we've agree to deprecate these JSON handling methods, from now on we may as well just use regular PHP json methods. RFC: silverstripe/silverstripe-framework#8424

@ScopeyNZ ScopeyNZ force-pushed the pulls/4.0/save-all-the-things branch 2 times, most recently from d575a38 to d44fdf2 Compare October 16, 2018 01:58
@ScopeyNZ ScopeyNZ force-pushed the pulls/4.0/save-all-the-things branch from d44fdf2 to 474d24f Compare October 16, 2018 04:31
@robbieaverill robbieaverill merged commit cd60cf4 into silverstripe:master Oct 16, 2018
@robbieaverill robbieaverill deleted the pulls/4.0/save-all-the-things branch October 16, 2018 15:01
This was referenced Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants