Settings: Better structure to settings page, and protect form changes #2544
Conversation
| 'You have unsaved changes. If you proceed, they will be lost.', | ||
| 'woocommerce-admin' | ||
| ); | ||
| return event.returnValue; |
There was a problem hiding this comment.
Where does this string get rendered?
Chrome
FF
There was a problem hiding this comment.
Those are built into the browsers #
| } | ||
|
|
||
| componentDidMount() { | ||
| window.addEventListener( 'beforeunload', this.warnIfUnsavedChanges ); |
There was a problem hiding this comment.
how about history.listen() for internal navigation?
There was a problem hiding this comment.
So I just tried this out, and it appears to have the same issue that I was experiencing with <Prompt /> i.e. if I hit cancel via a dialog thrown via history.block(), the address bar still updates. I'm guessing Prompt might just be wrapping this history module :)
There was a problem hiding this comment.
Thinking about this some more, the feature doesn't really belong on the settings screen as it's not a setting - it's a tool. Having said that, if it's a lot of work to add a new page I think this will do for now. We'll no doubt tweak this when we implement the new navigation.
I'm also wondering if there's a situation where someone would run this import more than once? Seems unlikely. So, should the feature be inaccessible once it's been run? If that's the case we might consider prompting folks to perform the import upon activation of wc-admin, and avoid sending them to a separate screen at all.
|
Some good feedback there @jameskoster - @LevinMedia might have some responses to the history of this being on the settings page. I think users might use the tool more than once, but I agree that it should probably live elsewhere in the long-run |
|
@psealock checked out the option, wasn't working for me - what do you think about getting this in as-is? |
One possible scenario is someone has chosen to import a subset of data (last 3 months) and wants to come back to the tool to import the rest. Or, maybe a user has imported historical data manually via sql dump and would like to import it to wc-admin after already having done so initially.
Nonetheless, this is a good idea. How about a notification? |
There was a problem hiding this comment.
Its a definite improvement as is. In the meantime, what if we change the language on the purple button from "Save Changes" to "Save Settings"? Would that be more implicit about the function of the button pertaining to all settings, not just the date ones? Or maybe a visual cue, such as more padding and center spacing?
Fixes #2449
This branch seeks to make the Analytics | Settings page a bit less confusing looking by adding in a proper section header to the Historical Data area, and it adds tracking to the state to set the page as
isDirtyif any changes are made, and subsequently warns users they have unsaved changes prior to navigating away.Before
After
Notes / Known Issues
Attempted to Core
I initially wanted to use WP Core's
<UnsavedChangesWarning />for this but it has a hardcoded state check that is tied into the editor's state. In a prior life, this core component supported manually setting the dirty state via a prop, but that was deprecated last october so I opted to emulate their approach here.Warning not shown when navigating via react-router
The warning of unsaved changes only works on the settings page if you are navigating to a non-wc-admin page, or if doing a hard refresh. I found this awesome approach on stackoverflow to use a built in tool from
react-routerto add support for this events, but after implementing I was hitting this bug in react-router - which even though using the<Prompt />component from react-router to stop navigation works, the address bar still updates and causes all sorts of janky-behavior.Saving settings prompt not shown
While working on this branch I also noticed that when I was saving new settings, the notice was not being shown, and the
isDirtystate was not being reset. At first I thought this was a bug in this branch, but indeed it exists in master. So I opened #2541 and added in a temporary fix here to optimistically set the form state back to clean when a save event is triggered. 46968fdDetailed test instructions:
Changelog Note:
Fix: Update layout of Settings Page and notify users when settings are not saved.