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

Cc/navigation prompts #1263

Merged
merged 7 commits into from Apr 25, 2022
Merged

Cc/navigation prompts #1263

merged 7 commits into from Apr 25, 2022

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Apr 20, 2022

Pre-Flight checklist

  • Screenshots and design review for any changes that affect layout or styling
  • Testing
    • Code is tested
    • Changes have been manually tested

What are the relevant tickets?

This is two of the three tasks in #991

What's this PR do?

This PR uses react-router's <Prompt /> component to prompt users for confirmation when navigation would cause dirty form changes to be discarded. A few other things are included. See comments and "Background" below.

How should this be manually tested?

  1. docker-compose restart watch to pickup changed frontend dependencies.

  2. Check closing the editing drawer:

    • Open any editing drawer, e.g., in "Pages", "Resources", "Videos"...wherever.
    • Closing the drawer (by clicking off the drawer or with "x") should prompt you for convfirmation if the form is dirty (has unsaved edits).
    • If you cancel when prompted, the drawer should not close.
    • Similar behavior if navigating to an external site / reloading page with unsaved changes. (But it's a different warning via beforeunload event. We have less control over the warning in this case.)

    Note: The "prompt for confirmation" dialog has changed and is now the browser builtin "confirm" prompt.

  3. Check that navigating away from unsaved changes elsewhere in the app prompts for confirmation. For example, on "Metadata", "Menus", or "Collaborators" pages

    • Check internal navigation (to other routes within the single page app)
    • Check external navigation (to routes outside the SPA)
  4. With a dirty "Metadata" form (or any other form for which the "Publish" button is still visible), click "Publish". The form changes should NOT be discarded.

    There will be subsequent work to do something more informative here like alert user "Hey, do you really want to publish with some unsaved changes?". But with this PR, at least the changes are not lost.

  5. Do any other navigation / form editing you feel like checking!

Any background context you want to provide?

On master we are only prompting users for dirty-form-discard-confirmation in two scenarios:

  1. closing the editing drawer with unsaved changes But not all form edits happen inside that drawer!
  2. when navigating to an external site

Ie, not when discarding dirty metadata / menu forms.

(1) happens via custom logic (the <ConfirmationModal /> component and associated useConfirmation hook) and (2) happens via beforeunload browser events. The beforeunload browser event is fired "when the window, the document and its resources are about to be unloaded."(mdn). So... not on navigation within the studio single-page app.

This PR replaces our custom <ConfirmationModal /> with React Router's <Prompt /> component. This change is enabled by the work Alice did tying editing to routes. Ie, before the editing drawer was tied to routes, we would not have been able to use , but now we can. So there's no need for a custom .

@codecov-commenter
Copy link

codecov-commenter commented Apr 20, 2022

Codecov Report

Merging #1263 (e667ccd) into master (bb9b0ad) will decrease coverage by 0.00%.
The diff coverage is 95.55%.

@@            Coverage Diff             @@
##           master    #1263      +/-   ##
==========================================
- Coverage   89.49%   89.48%   -0.01%     
==========================================
  Files         219      220       +1     
  Lines        9164     9158       -6     
  Branches     1707     1704       -3     
==========================================
- Hits         8201     8195       -6     
  Misses        850      850              
  Partials      113      113              
Impacted Files Coverage Δ
...lib/ckeditor/plugins/ResourceLinkMarkdownSyntax.ts 100.00% <ø> (ø)
static/js/lib/ckeditor/plugins/util.ts 98.24% <ø> (-0.02%) ⬇️
static/js/query-configs/websites.ts 99.00% <ø> (-0.01%) ⬇️
static/js/components/util/Prompt.tsx 86.66% <86.66%> (ø)
static/js/components/SingletonsContentListing.tsx 91.42% <100.00%> (-0.68%) ⬇️
static/js/components/SiteContentEditorDrawer.tsx 100.00% <100.00%> (+4.87%) ⬆️
...tatic/js/components/util/ConfirmDiscardChanges.tsx 100.00% <100.00%> (ø)
static/js/components/util/Spinner.tsx 100.00% <100.00%> (ø)
static/js/pages/SitePage.tsx 100.00% <100.00%> (ø)
static/js/test_util.ts 100.00% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb9b0ad...e667ccd. Read the comment docs.

return remove
}, [onBeforeUnload, when, message])

return <ReactRouterPromp {...otherProps} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One consequence of using react-router's is that confirmation is now requested via window.confirm, ie the browser's API. So the confirmation modal is now the browser's, not our customized version.

Before:
Screen Shot 2022-04-20 at 2 22 06 PM
After
Screen Shot 2022-04-20 at 2 23 07 PM

Update: I've customized this in the next PR toward #991. It will soon look like
Screen Shot 2022-04-21 at 2 48 41 PM

Comment on lines -88 to +87
"history": "^5.0.0",
"history": "^4.10.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use react-router v5 which uses history v4 internally. We should be using history 4, too. remix-run/history#804 (comment)

This also solves some typescript issues we've had.

Comment on lines -26 to -28
if (isLoading) {
return <div className="site-page std-page-body container">Loading...</div>
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This early return was causing unnecessary component mount/unmounts lower in the component tree. For example:

  1. Site is loaded, <SiteContentListing /> renders

  2. click publish, new site api request is sent

    Before: this <div className="site-page std-page-body container">Loading...</div> would replace the content listing, then a new content listing would be mounted after the API request is finished.

    Now: while the API request is in flight, the old content does not unmount (but a loading spinner shows atop it). When the API request finishes, the child component is updated (no unmount/remount, which was causing dirty form data to be lost).

@pdpinch pdpinch added this to To do in OCW Next via automation Apr 21, 2022
@gumaerc gumaerc self-assigned this Apr 22, 2022
Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

OCW Next automation moved this from To do to In progress Apr 22, 2022
@ChristopherChudzicki ChristopherChudzicki merged commit 2a5a13d into master Apr 25, 2022
OCW Next automation moved this from In progress to Done Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
OCW Next
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants