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

Admin UI: cleaned up react-router usage to match latest recommendations #3029

Merged
merged 11 commits into from
May 25, 2020

Conversation

Vultraz
Copy link
Contributor

@Vultraz Vultraz commented May 23, 2020

As of react-router 5.1 it's recommended to use children + hooks instead of render or component. 5.1 also included a fix to children so they no longer render when the route doesn't match.

These changes should make it easier for me to upgrade to v6 once it comes out.

  • Uses children for routes instead of render. I decided to stick with prop form since this is going to be renamed element in router v6 with children reserved for nested routes.
  • Moves the no-lists-defined page into its own file to be consistent with the other error/info pages.
  • Utilized useParams for route params

Minor code formatting:

  • HomePage: moved the DocTitle down into <main> and dropped the enclosing Fragment
  • ListNotFoundPage: simplified string construction

@changeset-bot
Copy link

changeset-bot bot commented May 23, 2020

🦋 Changeset is good to go

Latest commit: 1665f98

We got this.

This PR includes changesets to release 1 package
Name Type
@keystonejs/app-admin-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@timleslie timleslie left a comment

Choose a reason for hiding this comment

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

👍

@Vultraz
Copy link
Contributor Author

Vultraz commented May 24, 2020

Blah, why does this "Async callback was not invoked within the 5000ms" test failure keep happening :(

@timleslie
Copy link
Contributor

Yeah, I dunno, but it's a right pain in the butt :-( If you or someone else has some time to get to the bottom of it that'd be great.

@timleslie timleslie merged commit c35f9cd into keystonejs:master May 25, 2020
This was referenced May 25, 2020
@Vultraz Vultraz deleted the admin-ui-route-cleanup branch May 26, 2020 00:25
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.

2 participants