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

Updated with changes from original code base, fully migrated to react-router-dom-v5-compat #358

Closed
wants to merge 8 commits into from

Conversation

auan369
Copy link
Contributor

@auan369 auan369 commented Oct 19, 2024

Have included the most recent commits to the original code base and have migrated fully to react-router-dom-v5-compat

Copy link
Member

@petrvecera petrvecera left a comment

Choose a reason for hiding this comment

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

Looks great, I will test it :)

@@ -19,7 +20,8 @@ export const CommandersList = () => {
}>();

useEffect(() => {
document.title = `${commanderBase} - ${capitalize(race)}`;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had issues with the capitalize during the migration. If the original line is kept, I would face this error.
Screenshot 2024-10-19 at 4 45 50 PM

Copy link
Member

Choose a reason for hiding this comment

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

I see, you can fix this by passing there empty string like this ${capitalize(race || "")}

@petrvecera
Copy link
Member

Hi @auan369 it looks like there is some issue, it doesn't load for me

image

@auan369
Copy link
Contributor Author

auan369 commented Oct 19, 2024

Tr

Hi @auan369 it looks like there is some issue, it doesn't load for me

image

Trying it out on my side again and I have the same issue now as well. Seems like issue with Link in main-header.tsx and main-footer.tsx still persists.

Changing the import to import { Link } from "react-router-dom"; for main-header.tsx and main-footer.tsx should resolve the error

@auan369
Copy link
Contributor Author

auan369 commented Oct 19, 2024

Hi @petrvecera , I have discovered the issue with the Links. The update is to shift the CompatRouter component the above MainHeader and MainFooter components. Then we can use react-router-dom-v5-compat for all files.
Screenshot 2024-10-19 at 5 06 19 PM

@petrvecera
Copy link
Member

Hi @petrvecera , I have discovered the issue with the Links. The update is to shift the CompatRouter component the above MainHeader and MainFooter components. Then we can use react-router-dom-v5-compat for all files.

Nice, can you push this change? 👍

@auan369
Copy link
Contributor Author

auan369 commented Oct 19, 2024

Just sent the PR 👍

@petrvecera
Copy link
Member

closing this in favor of #359

FYI: you can always just update the original branch, no need to create a new one, you can also always do git push --force to force the change in the commits

@petrvecera petrvecera closed this Oct 22, 2024
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