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: confirmation when navigating from a form #3548

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mariojsnunes
Copy link
Collaborator

@mariojsnunes mariojsnunes commented May 15, 2024

PR Checklist

PR Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Developer experience (improves developer workflows for contributing to the project)

Description

Updated usePrompt to use useBlocker which should be more reliable.
I remember I had to do this change a few months ago on another project due to a chrome update.
Hopefully this will improve flaky tests.

Git Issues

Closes #

@mariojsnunes mariojsnunes requested a review from a team as a code owner May 15, 2024 07:57
Copy link

cypress bot commented May 15, 2024

4 failed and 2 flaky tests on run #5600 ↗︎

4 77 1 0 Flakiness 2

Details:

fix: confirmation when navigating from a form
Project: onearmy-community-platform Commit: f10f39aa6e
Status: Failed Duration: 04:45 💡
Started: May 15, 2024 9:50 AM Ended: May 15, 2024 9:55 AM
Failed  src/integration/howto/write.spec.ts • 1 failed test • ci-chrome

View Output Video

Test Artifacts
[How To] > [Create a how-to] > [Warning on leaving page] Test Replay Screenshots Video
Failed  src/integration/research/write.spec.ts • 3 failed tests • ci-chrome

View Output Video

Test Artifacts
[Research] > [Create research article] > [By Authenticated] Test Replay Screenshots Video
[Research] > [Create research article] > [Warning on leaving page] Test Replay Screenshots Video
[Research] > [Add a research update] > [By Owner] Test Replay Screenshots Video
Flakiness  settings.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test Artifacts
[Settings] > [Focus Member] > [Add a user pin] Test Replay Screenshots Video
Flakiness  howto/write.spec.ts • 1 flaky test • ci-chrome

View Output Video

Test Artifacts
[How To] > [Create a how-to] > [By Authenticated] Test Replay Screenshots Video

Review all test suite changes for PR #3548 ↗︎

@benfurber
Copy link
Member

@mariojsnunes I'll rerun the tests but this one looks related to your change?

@evakill evakill added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Jun 3, 2024
Copy link
Contributor

github-actions bot commented Jun 3, 2024

Visit the preview URL for this PR (updated for commit f10f39a):

https://onearmy-next--pr3548-fix-prompt-hmxhrjz2.web.app

(expires Wed, 03 Jul 2024 04:38:34 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59

@evakill evakill requested review from evakill June 3, 2024 15:14
Copy link
Collaborator

@evakill evakill left a comment

Choose a reason for hiding this comment

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

hey @mariojsnunes, the updates to the usePrompt hook look good.
could you give more context into the changes to src/pages/index.tsx? looks like we're pulling in some new components from react-router-dom, which is fine, but want to understand the motivation for the changes, as this impacts our entire routing infrastructure!

also, i noticed one weird behavior while testing - when you refresh from the how-to/create page, you get a 404 page not found
Screenshot 2024-06-03 at 3 21 39 PM

Comment on lines +4 to +14
// You can abstract `useBlocker` to use the browser's `window.confirm` dialog to
// determine whether or not the user should navigate within the current origin.
// `useBlocker` can also be used in conjunction with `useBeforeUnload` to
// prevent navigation away from the current origin.
//
// IMPORTANT: There are edge cases with this behavior in which React Router
// cannot reliably access the correct location in the history stack. In such
// cases the user may attempt to stay on the page but the app navigates anyway,
// or the app may stay on the correct page but the browser's history stack gets
// out of whack. You should test your own implementation thoroughly to make sure
// the tradeoffs are right for your users.
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this copied from documentation? if so, might be useful to also link the source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mod: HowTo 📰 Mod: Research 🔬 Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview
Projects
Status: No status
Status: 🆕 New
Development

Successfully merging this pull request may close these issues.

None yet

3 participants