Skip to content

Conversation

@vaibssingh
Copy link
Contributor

@vaibssingh vaibssingh commented Jul 11, 2023

Hey @JamieSlome , this should fix the start script and hopefully the build script too but I can't confirm either till we fix the issue with react-router-dom.

On starting the server and navigating to localhost:3000 I can see the API calls are being made and it tries to load the page till it encounters invalid imports from react-router-dom. So at least that's an improvement from yesterday's behaviour where it just gave 404 on opening.

Let me know how you want me to proceed with this. I am happy to help you with the updates regarding react-router-dom.

Thanks for your help.

Fix #192

@vaibssingh vaibssingh marked this pull request as draft July 11, 2023 21:35
@JamieSlome
Copy link
Member

@vaibssingh - awesome work 👍

I have started fixing up the react-router-dom problem in #185. I have pushed my changes to 185-fix-breakages-caused-by-migration-to-latest-react-router-dom6141 which I believe addresses all of the remaining issues. I will merge into main and you can merge in the latest changes into your branch then we can go from there.

@JamieSlome
Copy link
Member

For your reference: #200

@JamieSlome
Copy link
Member

@vaibssingh, a few conflicts for you there, sorry! The source of truth for what to keep comes from #200 😄

@JamieSlome JamieSlome self-requested a review July 12, 2023 09:11
@JamieSlome JamieSlome added the bug Something isn't working label Jul 12, 2023
@vaibssingh
Copy link
Contributor Author

Alright @JamieSlome! I will merge this and test this. Hopefully this should resolve the issues 🤞🏽

@vaibssingh
Copy link
Contributor Author

Hey @JamieSlome , updated the PR with the latest code. Checked and it seems to be working. npm run build & npm start both are working.

On UI, login screen opens up asking for credentials which I do not have so not able to test further than that.

@vaibssingh vaibssingh marked this pull request as ready for review July 12, 2023 10:17
Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

Please address the one comment, and then we are ready for lift-off! 🚀

@vaibssingh
Copy link
Contributor Author

@JamieSlome Done! 👍🏽

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

@JamieSlome JamieSlome merged commit 3902360 into finos:main Jul 12, 2023
coopernetes pushed a commit to coopernetes/git-proxy that referenced this pull request Oct 13, 2023
fix: rename files and use vite config in start script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate from react-scripts to Vite ⚡️

2 participants