-
Notifications
You must be signed in to change notification settings - Fork 89
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
WIP > feat: twap form styling WIP #2785
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@fairlighteth I'll take care of the hard coded value |
Follow up draft PR as a WIP on further styling: #2796 |
@fairlighteth done on #2797 I also did a bit of refactor to make it more generic |
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.
Hey @fairlighteth , great job!
Some issues that I faced in the PR:
- I get a crash when I open the app in iOS device (iPhone 12 mini), navigate to the Advanced orders page and connect to MM using WCv1
- In iOS (real device) in the light mode color of an amount in the To field is different:
- When I enter a decimal value into the Price protection field, the value is cropped
- Price protection field does not look good in a mobile view
- It would be great to add upwards and downwards arrows into the Parts field
Thanks!
* refactor: `ReactNode` already has `string` inside it * feat: new optional params to ExecutionPrice * refactor: make TradeWidgetField and TradeNumberInput more generic * feat: calculate limit price and pass it down to price protection row * feat: pass isInverted state onto price row * feat: calculate buyAmount outside of twapOrderAtom * feat: use buyAmount from its own atom twapOrder is null when the account isn't set * feat: show `0` rather than `-` when there's no limit price
… twap-form-styling-1 # Conflicts: # src/modules/trade/pure/TradeNumberInput/index.tsx # src/modules/twap/containers/TwapFormWidget/index.tsx # src/modules/twap/containers/TwapFormWidget/tooltips.tsx # src/modules/twap/state/twapOrderAtom.ts
Co-authored-by: Leandro <[email protected]>
placeholder = '0', | ||
decimalsPlaces = 0, | ||
min, | ||
max = 100_000, |
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.
why max is 100K?
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.
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.
a bit arbitrary, but ooook
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.
100% arbitrary yes!
Open to suggestions
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.
Is there any reason why we would ever want to have an order with more than 100k parts?
* feat: twap form styling WIP * feat: twap form styling WIP * feat: twap form styling WIP * feat: twap form styling WIP * feat: twap form styling WIP * feat: twap form styling WIP * feat: twap form styling WIP * feat: twap form styling WIP * Fix code style issues with Prettier * feat: update styles * feat: update styles * feat: update styles * Fix code style issues with Prettier * feat: styling updates * feat: styling updates * feat: styling updates * feat: styling updates * Fix code style issues with Prettier * feat: styling updates * feat: styling updates * feat: fix custom date style * feat: safar text color fix * feat: fix styles * feat: fix styles * feat: fix styles --------- Co-authored-by: Lint Action <[email protected]>
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.
Hey @fairlighteth , great job!
Approving the PR in order not to block the release.
However, still there is 1 nitpick related to the tooltip icon in a mobile view
LMK please if I need to create a separate issue for this.
Thanks
Also there should be enough margin in between the InlineBanners |
@fairlighteth , thanks! LGTM! |
Summary
Import Todo's: