Skip to content

Conversation

@kyletsang
Copy link
Member

There were some breaking changes from beta to stable, so this PR fixes that.

Wasn't sure if we actually needed to render a Route inside the LinkContainer. It was causing errors in v6 stable. I think the hooks provided by React Router should be enough. I tested using the visual tests page and everything looks like it works.

@kyletsang kyletsang requested a review from mxschmitt November 18, 2021 06:28
@Coolfeather2
Copy link

Coolfeather2 commented Nov 18, 2021

Still not working for me inside my project, or in the visual test page
Inside the visual test page, no links are there. (At least the nav page is not working)

@kyletsang
Copy link
Member Author

Still not working for me inside my project, or in the visual test page Inside the visual test page, no links are there. (At least the nav page is not working)

You downloaded the branch this PR is referencing and npm installed?

Copy link
Member

@mxschmitt mxschmitt left a comment

Choose a reason for hiding this comment

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

LGTM, but next time split applying formatting changes with changing logic. Thanks!

@Coolfeather2
Copy link

  1. git cloned
  2. checked out react-router-v6-stable
  3. ran npm install
  4. ran npm run build
  5. ran npm run visual-test
    image
node -v
v14.15.4
npm -v
6.14.10

@Coolfeather2
Copy link

Additionally, when packaging this up, or using npm link it doesn't work

package.json is looking for ./lib/index.js, but ./lib/ReactRouterBootstrap.js is being built

@kyletsang
Copy link
Member Author

Yeah the NavItem thing should be fine. There's supposed to be a NavLink underneath that gives it a little bit more styling

I'm going to merge this for now and address the build in another follow-up PR.

@kyletsang kyletsang merged commit d4df5a6 into master Nov 19, 2021
@kyletsang kyletsang deleted the react-router-v6-stable branch November 19, 2021 01:11
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