-
Notifications
You must be signed in to change notification settings - Fork 710
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
Fixes #1265 Updated example for react-router v6.4 #1266
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for testing-library ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@@ -3,7 +3,159 @@ id: example-react-router | |||
title: React Router | |||
--- | |||
|
|||
This example demonstrates React Router v6. For previous versions see below. | |||
This example demonstrates React Router v6.4 and above. For previous versions see below. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not very familiar with this, but I assume createBrowserRouter
is added in v6.4?
} from 'react-router-dom'; | ||
|
||
// Method to introduce an artificial delay | ||
export function sleep(n = 500) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clean this example up please?
sleep
doesn't really add any value here, it's just introduces clutter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this is a modified form of the example in react router examples source code I wanted this to be close to the original doc and hence left it.
await sleep(); | ||
let formData = await request.formData(); | ||
let name = formData.get('name'); | ||
console.log(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
} | ||
|
||
// Action to get user input | ||
export async function aboutAction({ request }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can also be left out, or there should be a test that verifies its behavior.
expect(screen.getByTestId('location-display')).toHaveTextContent(route); | ||
}); | ||
``` | ||
Refer to [this working example](https://stackblitz.com/edit/vitejs-vite-dnutcg?file=src%2FApp.test.jsx,src%2FApp.jsx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because other examples don't have a stackblitz I was thinking on removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Te reason for having that was, I couldn't get other examples to run without making code changes
Would be nice to have updated examples for react router on the website. |
@vhakulinen Feel free to move forwards with this PR/create a different PR :) |
Unfortunately, I don't have the bandwidth to do that at the moment. (I hope my comment didn't come off as rude, I really appreciate all the work done by you all) |
On the contrary, I want this PR to be pushed also. |
Added a section with an example to use
testing-library
withreact-router
v6.4
and above