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

Fix No Warning on Browser Button Navigation(Fixes #2403) #2529

Merged

Conversation

Algentile
Copy link
Contributor

@Algentile Algentile commented Dec 9, 2019

Test Instance can be found https://local-iodide.herokuapp.com/ for QA. This is a change to resolve #2403.

Pull Request checklist

  • Documentation: If this feature has or requires documentation, the relevant docs have been updated.
  • Changelog: This PR updates the changelog with any user-visible changes.
  • Tests: This PR includes thorough tests or an explanation of why it does not

@Algentile
Copy link
Contributor Author

Here we just check for push state in history if a back navigation, or page refresh is called from the browser when a change is made. This can also be triggered by catching the overall onbeforeunload event.

@codecov
Copy link

codecov bot commented Dec 9, 2019

Codecov Report

Merging #2529 into master will decrease coverage by 1.23%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2529      +/-   ##
==========================================
- Coverage   71.08%   69.84%   -1.24%     
==========================================
  Files         238      223      -15     
  Lines        5841     5515     -326     
  Branches      935      887      -48     
==========================================
- Hits         4152     3852     -300     
+ Misses       1670     1644      -26     
  Partials       19       19
Impacted Files Coverage Δ
...als/file-modal/manage-file-sources/in-progress.jsx 33.33% <0%> (-66.67%) ⬇️
src/reps/components/data-table-rep.jsx 12.96% <0%> (-62.97%) ⬇️
src/reps/components/rep-info-requestor.jsx 20.83% <0%> (-50%) ⬇️
src/shared/components/buttons.jsx 34.78% <0%> (-30.44%) ⬇️
src/reps/components/value-summary.jsx 69.69% <0%> (-25.9%) ⬇️
src/reps/serialization/get-error-stack-summary.js 26.66% <0%> (-22.23%) ⬇️
src/reps/serialization/value-summary-serializer.js 94.23% <0%> (-1.93%) ⬇️
src/reps/__test_helpers__/reps-test-value-cases.js 66.92% <0%> (-0.77%) ⬇️
server/notebooks/serializers.py 94.11% <0%> (-0.17%) ⬇️
server/notebooks/api_views.py 98.91% <0%> (-0.03%) ⬇️
... and 27 more

@bcolloran
Copy link
Contributor

this works for me, thanks @Algentile. I've been thinking about the interaction model, and I think we can continue to refine it along the lines of what i mentioned in this this comment #2403 (comment) but this is a great step in preventing people from losing work!

@bcolloran bcolloran merged commit 0796675 into iodide-project:master Dec 10, 2019
@Algentile Algentile deleted the 2403_fix_back_button_intercept branch December 22, 2019 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigating between explore and report page with back button does not work
2 participants