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

Site Settings: Mark fields as changed when changing in wrapSettingsForm #10690

Merged
merged 1 commit into from Jan 17, 2017

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Jan 17, 2017

This PR fixes #10670, where it was reported by @drw158 that the Settings: Discussion form does not alert users when changing to another Settings tab with unsaved settings.

This appears to have been introduced in #9899, which reduxified the Discussion form.

The solution proposed in this PR is to mark each field as changed when handling a change in any of the fields.

To test:

  • Checkout this branch
  • Go to /settings/discussion/$site, where $site is one of your sites.
  • Change any setting, do not click "Save Changes".
  • Switch to another tab and verify you're presented with an alert that prevents you from losing unsaved changes.

/cc

@matticbot
Copy link
Contributor

@tyxla tyxla added [Feature] Site Settings All other general site settings. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 17, 2017
@tyxla tyxla self-assigned this Jan 17, 2017
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I made a similar change here #10691 but yours is more complete. I only updated the handleToggle callback because most of the time when we use onChangeField or handleRadio, we're relying on regular HTML inputs which should trigger the onChange callback of the form (But I guess these methods could be called in other places, so it's fine to keep these calls to markChanged.

Though, I don't think this was introduced by the reduxification itself. I guess when we were using FormCheckbox the form onChange where being triggered since we were using regular HTML inputs, but it's not the case anymore with toggles.

BTW, This is testing well! 👍

@tyxla tyxla added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 17, 2017
@tyxla
Copy link
Member Author

tyxla commented Jan 17, 2017

Thank you for the review, 🐑 ing.

@tyxla tyxla merged commit 7d16df2 into master Jan 17, 2017
@tyxla tyxla deleted the fix/wrap-settings-form-markchanged branch January 17, 2017 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Site Settings All other general site settings.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Settings: setting toggle not saved when navigating away
3 participants