-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add frontend #4
Add frontend #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You didn't internationalize the texts, maybe first create the lib in the ui kit with i18n file and then create translation keys here for this project
"@massalabs/react-ui-kit": "0.0.4-dev.20240429153446", | ||
"@massalabs/wallet-provider": "^2.0.1-dev.20240423112601", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"@massalabs/react-ui-kit": "0.0.4-dev.20240429153446", | |
"@massalabs/wallet-provider": "^2.0.1-dev.20240423112601", | |
"@massalabs/react-ui-kit": "0.0.4-dev", | |
"@massalabs/wallet-provider": "^2.0.1-dev", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run npm i
after to update lock file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea as we often break things in the packages while they have in the dev version and so we can rely on their version following semver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya but the version upgrade only explicitely when you npm update @massalabs/wallet-provider
this works well for all other projects, don't run npm update
and all will be good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone clone the repository and use yarn for example to launch his project it could fetch updated packages and as we don't follow semver in the dev version it could be packages with breaking changes (there is a command in yarn to use the package-lock.json but nearly nobody use it).
My point is that, the semve ensure we don't break when using "^" on version and that's not the case with our dev version and so we shouldn't use this syntax.
frontend/src/lib/connectMassaWallets/components/ConnectedAccount.tsx
Outdated
Show resolved
Hide resolved
@Thykof What do you mean by "create the lib in the UI kit" ? |
Co-authored-by: Nathan Seva <[email protected]>
I meant exactly what you describe here massalabs/ui-kit#428! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few small comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
The frontend is static for now (except wallet selection) and there is no CI. I will add them after but I would like to have a review on this first version as it's my first frontend project.
The designs are available here : https://www.figma.com/file/LSaOibVDFb1reF6FePVn5l/Massastation?type=design&node-id=8344-20414&mode=design&t=KznEUhbhluUoyGKF-0
I made some modification to match more our existing product to match more the coin-vester I will discuss this with Lucas that's not really the point of the review for now.