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

Fixes #1265 Updated example for react-router v6.4 #1266

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

Conversation

maruhgar
Copy link

Added a section with an example to use testing-library with react-router v6.4 and above

@netlify
Copy link

netlify bot commented Jun 23, 2023

Deploy Preview for testing-library ready!

Name Link
🔨 Latest commit 4213d97
🔍 Latest deploy log https://app.netlify.com/sites/testing-library/deploys/649564ef0b4d3800086babe4
😎 Deploy Preview https://deploy-preview-1266--testing-library.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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.
Copy link
Member

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) {
Copy link
Member

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.

Copy link
Author

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);
Copy link
Member

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 }) {
Copy link
Member

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)
Copy link
Member

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?

Copy link
Author

@maruhgar maruhgar Aug 14, 2023

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

@vhakulinen
Copy link

Would be nice to have updated examples for react router on the website.

@MatanBobi
Copy link
Member

@vhakulinen Feel free to move forwards with this PR/create a different PR :)

@vhakulinen
Copy link

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)

@MatanBobi
Copy link
Member

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.

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.

4 participants