-
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 Nabla UI #326
Add Nabla UI #326
Conversation
…ontract-library # Conflicts: # src/shared/useContractWrite.ts
✅ Deploy Preview for rococo-souffle-a625f5 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@nejcm I see that you are still adding commits to this PR. Is this already ready to be reviewed or still work in progress? |
I had to fix the failing testa and refactor some folders. This is ready to merge. I dont have perms to do so though. |
Okay, I will review this ticket and test locally. The PR is quite massive, so this will take a little time. |
So it looks like a lot mostly because of the graphql generated files being changed. If you need and info or help from my side I am available. |
src/components/nabla/Pools/Backstop/AddLiquidity/useAddLiquidity.ts
Outdated
Show resolved
Hide resolved
src/components/nabla/Pools/Backstop/WithdrawLiquidity/useBackstopDrain.ts
Show resolved
Hide resolved
src/components/nabla/Pools/Backstop/WithdrawLiquidity/useWithdrawLiquidity.ts
Outdated
Show resolved
Hide resolved
1fc6f8c
to
0dc1533
Compare
src/components/nabla/Pools/Backstop/WithdrawLiquidity/useWithdrawLiquidity.ts
Show resolved
Hide resolved
src/components/nabla/Pools/Backstop/WithdrawLiquidity/useWithdrawLiquidity.ts
Show resolved
Hide resolved
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.
Generally, I think the code is okay. It's not great but mostly because the implementation of the Nabla UI happened in parallel but in isolation so we now have a confusing structure and some duplicated components that we should streamline.
However, I think we should rather address the problem of duplicate components and bad structure of our code files in a separate PR with the sole purpose of fixing those things. Then we can also discuss and agree how this perfect structure should look like.
src/components/nabla/Pools/Swap/AddLiquidity/useAddLiquidity.ts
Outdated
Show resolved
Hide resolved
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.
PR looks good 😎 🚀 , a few small changes to review if they should be applied. I think we are also repeating the code of element in the application, and we should someday create a single source of truth for using inputs in our application, because otherwise we may be repeating code unnecessarily and sometimes this can lead to errors when we miss some validation in one input
Fully agree! |
I addresses add issues in the PR and bugs found during QA. I also added proper token logos and there are no merge conflicts left. |
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.
Nice, thanks for putting in the effort to fix all the remaining issues @TorstenStueber 👍
Let's merge this and create a follow-up ticket for refactoring.
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.
Perfect ✅
@TorstenStueber is backstop pool withdrawal fixed? |
@prayagd Yes. |
This PR swaps the polkadot-api contract calls with pendulum-solang library.