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

feat(router): add router useBlocker hook #10873

Merged
merged 30 commits into from
Jul 23, 2024

Conversation

xmaxcooking
Copy link
Contributor

@xmaxcooking xmaxcooking commented Jun 24, 2024

Successor of: #9843

Can be used like this:

const PostForm = (props: PostFormProps) => {
  const form = useForm<FormPost>()
  const blocker = useBlocker({ when: form.formState.isDirty })

  return (
    <div className="rw-form-wrapper">
      <Form formMethods={form} onSubmit={onSubmit} error={props.error}>
        State {blocker.state}
        {blocker.state === 'BLOCKED' ? (
          <div>
            <button type="button" onClick={() => blocker.confirm()}>
              Confirm
            </button>
            <button type="button" onClick={() => blocker.abort()}>
              Abort
            </button>
          </div>
        ) : null}
       ...
      </Form>
    </div>
   )
}

Tab close, Browser Close and Navigation with the redwood router components and functions can be blocked.

The only thing not working is the interception of the browser back button.

I found possible solutions to block the navigation back too, but not without losing any user changes and/or messing with the history.

React-router defined the situation like this: https://github.com/remix-run/react-router/blob/aacc2b94088835d1247bdf3a3f883f74cc3570a0/decisions/0001-use-blocker.md?plain=1#L37C1-L37C127

Blocking POP navigations is different - since we don't know about them via popstate until _after_ the URL has been updated

Currently the redwood router and the active page are coupled to the real location (URI) and it would require a refactoring to couple it with something like a location proxy instead which could be kept in sync with the history and location.

Any thoughts? Would a refactoring be worth it? Or is this implementation with a missing piece of functionality good enough?

@xmaxcooking xmaxcooking marked this pull request as draft June 24, 2024 18:48
@xmaxcooking
Copy link
Contributor Author

xmaxcooking commented Jun 24, 2024

Changed to draft until discussed. also may need some tests and docs.

@Tobbe
Copy link
Member

Tobbe commented Jun 25, 2024

Thanks for picking this back up again @xmaxcooking 🙏

I'll have to read back up on our previous discussions before I can provide much meaningful input here. I'll try to get to it this week, please ping me if I don't

@Tobbe Tobbe self-assigned this Jun 25, 2024
@xmaxcooking xmaxcooking marked this pull request as ready for review June 25, 2024 16:02
@xmaxcooking xmaxcooking marked this pull request as draft June 25, 2024 16:02
@Tobbe
Copy link
Member

Tobbe commented Jun 30, 2024

I haven't forgotten about his PR, but I have some tight deadlines coming up (one for Monday, one for Tuesday and finally one for Wednesday). I've set a reminder to myself to check back on this PR on Thursday

Copy link
Member

@Tobbe Tobbe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Overall I like this :) Just a few things:

  • Please fix all the errors and warnings
  • Please add tests
  • Take a look at my two comments and see what you think about them

packages/router/src/useBlocker.ts Outdated Show resolved Hide resolved
packages/router/src/link.tsx Outdated Show resolved Hide resolved
@xmaxcooking
Copy link
Contributor Author

Thanks for the review! I'm going to polish this next week and then change it from draft to ready.

@xmaxcooking xmaxcooking marked this pull request as ready for review July 11, 2024 14:19
@xmaxcooking xmaxcooking requested a review from Tobbe July 11, 2024 14:20
Copy link
Member

Tobbe commented Jul 11, 2024

Awesome @xmaxcooking! I’m taking today off, but will look when I get a chance

packages/router/src/link.tsx Outdated Show resolved Hide resolved
@Tobbe Tobbe added the release:feature This PR introduces a new feature label Jul 15, 2024
@Tobbe Tobbe added this to the next-release milestone Jul 15, 2024
@xmaxcooking
Copy link
Contributor Author

Added the playwright test and removed the link changes👍

@Tobbe Tobbe merged commit 110a101 into redwoodjs:main Jul 23, 2024
46 checks passed
@Tobbe
Copy link
Member

Tobbe commented Jul 23, 2024

Thank you so much for the work you put into this feature @xmaxcooking! Very much not taken for granted 🙏

Josh-Walker-GM added a commit that referenced this pull request Jul 27, 2024
**Problem**
The rebuild-test-project-fixture script was broken locally - and
probably on CI too.

**Changes**
1. Move generation of user sdl ahead of dbauth setup
2. Updates general version bumps in fixure
3. Updates for fragment type fix (on or around #10825
4. Updates for useBlocker in contact us form #10873
5. Updates to remove unused imports and have lint check complete
successfully
@Josh-Walker-GM Josh-Walker-GM modified the milestones: next-release, v8.0.0 Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:feature This PR introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants