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

chore: use react-router new routing approach RouterProvider #3761

Merged
merged 9 commits into from
Jul 12, 2024

Conversation

mariojsnunes
Copy link
Contributor

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

https://github.com/remix-run/remix/blob/bbead33bf530be030d16a1466b52cd99d077382a/decisions/0005-remixing-react-router.md#react-router-api-surface
Use the newer and recommended routing approach to make it easier to migrate to Remix later.

@mariojsnunes mariojsnunes requested a review from a team as a code owner July 11, 2024 10:19
@mariojsnunes
Copy link
Contributor Author

I'll wait for someone else to take a look. And also test on the preview website.

@benfurber benfurber self-assigned this Jul 11, 2024
@benfurber benfurber added the Review allow-preview ✅ Has received manual check for malicious code and can be safely built for preview label Jul 11, 2024
Copy link
Contributor

github-actions bot commented Jul 11, 2024

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

https://onearmy-next--pr3761-chore-react-router-e-hkf1dsp1.web.app

(expires Sun, 11 Aug 2024 14:27:45 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59

Copy link
Member

@benfurber benfurber left a comment

Choose a reason for hiding this comment

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

Could deffo refactor the routing setup but not the focus of this PR. Will do a quick manual check on the preview build too.

Copy link

cypress bot commented Jul 11, 2024

Passing run #6011 ↗︎

0 71 1 0 Flakiness 0

Details:

fix: confirmation modal and tests
Project: onearmy-community-platform Commit: a387fd898e
Status: Passed Duration: 04:13 💡
Started: Jul 12, 2024 10:37 AM Ended: Jul 12, 2024 10:41 AM

Review all test suite changes for PR #3761 ↗︎

@benfurber
Copy link
Member

A basic click through confirmed all routes working as before.

@mariojsnunes
Copy link
Contributor Author

Found that after this change, confirmation on leaving the page no longer works.
Added a fix that also improves the confirmation to use our modal instead of the browser default one. (easier to test too)

Besides that, found a few more instances where forms were being submitted by clicking other buttons, which were breaking some tests. (because the default type of buttons is submit, not button)

@mariojsnunes mariojsnunes marked this pull request as draft July 12, 2024 07:55
@mariojsnunes
Copy link
Contributor Author

found more issues when saving forms

@mariojsnunes mariojsnunes marked this pull request as ready for review July 12, 2024 10:30
@mariojsnunes
Copy link
Contributor Author

mariojsnunes commented Jul 12, 2024

Note the change on UnsavedChangesDialog. The behaviour no longer relies on uploadComplete state. There were tricky rendering issues that caused that observable to not update on this component context.
Now it relies only on the dirty and submitSuccess variables.
For submitSuccess to work as intended, had to add await to the onSubmit calls.

@benfurber
Copy link
Member

Awesome, this looks like really good work @mariojsnunes.

@benfurber benfurber merged commit cc1fdd0 into master Jul 12, 2024
21 of 22 checks passed
@benfurber benfurber deleted the chore/react-router-elements branch July 12, 2024 12:01
@onearmy-bot
Copy link
Collaborator

🎉 This PR is included in version 1.199.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants