-
Notifications
You must be signed in to change notification settings - Fork 2k
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: only update clean fields with updated poll data #1167
Conversation
I don't think we should disable the refreshing of data completely. That means that once I hit the settings page, any changes that may happen externally won't be reflected. It's specially noticeable if you open two tabs with settings and in the second one you change the site title. After the poller kicks in, you'll see that the sidebar in the first site updates the selected site with the new title, however, the from field would still have keep the old one. I think a better solution would be to have the notion of "dirty" for the fields, and leave alone fields that the user may have changed during a poller cycle update. |
Yeah, I agree with this. |
278123b
to
260a661
Compare
7a65179
to
c163086
Compare
c163086
to
b293f2b
Compare
@@ -131,7 +139,9 @@ module.exports = { | |||
|
|||
notices.clearNotices( 'notices' ); | |||
|
|||
this.setState( { submittingForm: true } ); | |||
//for dirtyFields, see lib/mixins/dirty-linked-state | |||
this.setState( { submittingForm: true, dirtyFields: [] } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the clearing of dirtyFields
down to the success case below? If there's some transient internet hiccup and you're in the error case where you just need to resubmit, I think we'd still want the dirtyFields
to be retained until the form is successfully submitted. Otherwise your changes would potentially be cleared out while you are in that error state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good catch, I'll only clear that out in the success case
This is great. I had one suggestion, and if you agree, I think this is good to 🚢 once that change is made. I wonder if this would be a useful module to open source separately and publish via npm? I wouldn't do so in this PR, but I think we need some sort of strategy around open sourcing smaller modules. I think this is what @retrofox was talking about in our hangout last week. |
this.setState( { submittingForm: true } ); | ||
//for dirtyFields, see lib/mixins/dirty-linked-state | ||
this.setState( { submittingForm: true, dirtyFields: [] } ); | ||
|
||
site.saveSettings( this.state, function( error ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll also add an omit( this.state, 'dirtyFields' )
so we don't send over an unneeded field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 likewise, good catch
It's a pretty good candidate for it. Sounds like we might want to add in some generators to make publishing npm packages easier? Like boilerplate babel config/testing setup. |
…ly update clean fields with updated poll data.
b293f2b
to
83aba4d
Compare
Site settings: only update clean fields with updated poll data
Fixes #43 #40 and #42 where unsaved changes to site settings could be wiped out on rerender. This is triggered by SitesList polling periodically via the /me/sites endpoint. Note that this affects all settings, not just the site tagline.
Testing Instructions
Expected: Your unsaved changes are still there.
cc @hoverduck @rralian