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

Custom Fields option: Add confirmation step #15688

Merged
merged 12 commits into from Aug 6, 2019

Conversation

@marekhrabe
Copy link
Contributor

commented May 16, 2019

Fixes #15320.

"Custom Fields" in Options modal requires a full page reload in order to toggle the option. Currently, this happens instantly after clicking the checkbox.

This PR changes the flow to add an inline confirmation step:

see the latest version #15688 (comment)

@marekhrabe marekhrabe requested review from talldan and youknowriad as code owners May 16, 2019

@marekhrabe marekhrabe force-pushed the try/custom-fields-toggle-deferred branch from ff0a5a3 to 77d3e11 May 25, 2019

@marekhrabe marekhrabe changed the title Custom Field check box - try using DeferredOption Custom Field check box - try using confirmation button May 25, 2019

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

Update:

This PR shows additional message about a reload and a confirmation button to proceed. This additional UI only shows when a reload action is required and disappears when you change your mind and toggle the option back without a confirmation. It is additionally announced with spoken messages to differentiate the tickbox from all others which save options instantly.

It's not well styled and polished at this point. Let's figure out a proper interaction first.

Question: Do spoken messages make sense here? If so, what would their formulations be? I'm sure I haven't came up with the most ideal copy.

If we confirm this approach is a way forward, I can take a look at styling, making string translatable etc…

@noisysocks

This comment has been minimized.

Copy link
Member

commented May 27, 2019

I can't answer the question about using speak(), but functionally I think this is the right approach!

I added the Needs Design label so that the design team can offer some suggestions. Personally I'd advocate for something relatively inline and unobtrusive.

Screen Shot 2019-05-27 at 11 22 06

(The link would be a <Button> with isLink.)

EDIT: @mapk shared a much nicer design in #15320 (comment).

@karmatosed karmatosed requested a review from mapk May 28, 2019

@mapk

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Oh dang, turns out I added a redesign to the issue and not the PR. In case people don't want to click around, here's the proposed redesign.

Screen Shot 2019-05-28 at 6 58 44 PM

I'm not entirely sure if the notification should be blue or yellow. Any thoughts on that?

@mapk

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Okay, because this is more of a warning notification, let's go with a yellow version.

Screen Shot 2019-05-29 at 10 07 20 AM

@afercia

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2019

Discussed during today's accessibility bug scrub. Quick feedback: if there’s text with relevant information and if the button says “Save and Reload” there’s no need for a spoken message.

@noisysocks noisysocks self-assigned this Jul 4, 2019

@noisysocks noisysocks force-pushed the try/custom-fields-toggle-deferred branch from 77d3e11 to a51bf94 Jul 5, 2019

@talldan
Copy link
Contributor

left a comment

I had one big question about 'Save & Reload', so haven't approved this, but generally the code looks good.

I know that previously clicking the custom fields option didn't save the post, but I think the messaging now will lead to some expecting that to happen.

@karmatosed

This comment has been minimized.

Copy link
Member

commented Jul 24, 2019

Can I get confirmation that this does 'save and reload'? I am reading comments and wondering if that is the case @noisysocks and @talldan. If it doesn't actually do that saying so is a little unexpected.

@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2019

I know that previously clicking the custom fields option didn't save the post, but I think the messaging now will lead to some expecting that to happen.

I believe the "save" refers to the setting. It's saving the setting and reloading the page. But it does cause confusion when the help text says that "A page reload is required" and then the button implies a "Save and load" as if referring to the page.

Maybe we can adjust the language of the button? Here are some ideas:

Submit
Agree
Acknowledge
Reload
Do it

Any other thoughts?

@talldan

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

Agree that adjusting the wording would be a good compromise to get this merged. I like @marekhrabe's suggestions in the comment above, or just shortening it to 'Reload' / 'Reload now'.

Could it also be worth adding something to the main part of the message? e.g. 'A page reload is required for this change, please save any changes to your content before reloading.'

marekhrabe and others added 3 commits Jul 25, 2019
update description
Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>
@mapk

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

"Enable & Reload" or "Disable & Reload"

Oh, I missed these. I prefer them over my suggestions. Thanks!

Could it also be worth adding something to the main part of the message? e.g. 'A page reload is required for this change, please save any changes to your content before reloading.'

If handling the save of the document automatically before reloading gets tricky, this seems like the right solution. And if they miss that text and trigger the reload anyways, alerting them with an "Are you sure" message as @marekhrabe suggested works too.

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2019

"Enable & Reload" or "Disable & Reload"

I have implemented these in 8320d22

And if they miss that text and trigger the reload anyways, alerting them with an "Are you sure" message as @marekhrabe suggested works too.

"Are you sure" functionality is done on a global level and it is already triggering for this option.

I have added an extra sentence to the info message about saved content.

Current State

Screenshot 2019-07-27 at 09 37 12

Screenshot 2019-07-27 at 09 37 01

With unsaved content:

Screenshot 2019-07-27 at 09 37 31

marekhrabe added 2 commits Jul 27, 2019

@gziolo gziolo removed this from the Gutenberg 6.2 milestone Jul 30, 2019

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

All tests now account for new labels. Are we good to go with this state #15688 (comment)?

@marekhrabe marekhrabe requested a review from talldan Aug 1, 2019

@noisysocks
Copy link
Member

left a comment

The code changes look good, and this tests well. Thanks for the assist @marekhrabe!

I'll leave it for @mapk to give the final say on whether the design and copy are ready to be merged.

marekhrabe and others added 2 commits Aug 1, 2019
simplify condition for determining label
Co-Authored-By: Robert Anderson <robert@noisysocks.com>
@mapk

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

I gave it another go. The added sentence looks great! Thanks everyone for helping out with this one. :shipit:

@mapk
mapk approved these changes Aug 6, 2019
Copy link
Contributor

left a comment

:shipit: Looks good!

@marekhrabe marekhrabe added this to the Gutenberg 6.3 milestone Aug 6, 2019

@marekhrabe

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

This PR has truly been a journey 😀Thanks everybody!

@marekhrabe marekhrabe merged commit c5d573a into master Aug 6, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@marekhrabe marekhrabe deleted the try/custom-fields-toggle-deferred branch Aug 6, 2019

gziolo added a commit that referenced this pull request Aug 29, 2019
Custom Fields option: Add confirmation step (#15688)
* Add confirmation step to Custom Fields option

Add a confirmation step to the Custom Fields option so that the page
doesn't reload without warning.

Co-authored-by: Marek Hrabe <marekhrabe@me.com>

* Update E2E tests for new Custom Fields setting flow

* Remove notice in favor of plain text.

* Custom Fields option: Adjust margin to align text

* update description

Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>

* simplify change handler

* add dynamic button based on the next state to be saved

* add notice about saved content and make sure it won't expand modal

* update snapshot with new button label and confirm msg

* make button label in test dynamic

* simplify condition for determining label

Co-Authored-By: Robert Anderson <robert@noisysocks.com>

* use willEnable as the prop name
gziolo added a commit that referenced this pull request Aug 29, 2019
Custom Fields option: Add confirmation step (#15688)
* Add confirmation step to Custom Fields option

Add a confirmation step to the Custom Fields option so that the page
doesn't reload without warning.

Co-authored-by: Marek Hrabe <marekhrabe@me.com>

* Update E2E tests for new Custom Fields setting flow

* Remove notice in favor of plain text.

* Custom Fields option: Adjust margin to align text

* update description

Co-Authored-By: Daniel Richards <daniel.richards@automattic.com>

* simplify change handler

* add dynamic button based on the next state to be saved

* add notice about saved content and make sure it won't expand modal

* update snapshot with new button label and confirm msg

* make button label in test dynamic

* simplify condition for determining label

Co-Authored-By: Robert Anderson <robert@noisysocks.com>

* use willEnable as the prop name
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.