-
Notifications
You must be signed in to change notification settings - Fork 315
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
Feat/live 3533 swap provider list #1225
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ
|
π¦ Changeset detectedLatest commit: e8e0273 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Screenshots: β
There are no changes in the screenshots for this PR. If this is expected, you are good to go. |
0435e2a
to
776e11a
Compare
Codecov ReportBase: 85.64% // Head: 43.84% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #1225 +/- ##
============================================
- Coverage 85.64% 43.84% -41.81%
============================================
Files 12 609 +597
Lines 209 25501 +25292
Branches 38 6947 +6909
============================================
+ Hits 179 11180 +11001
- Misses 30 14275 +14245
- Partials 0 46 +46
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. β View full report at Codecov. |
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.
Good job overall but the component layout regarding the new rates list needs to be updated. It is currently sub optimal and constantly re-rendered (every second), triggering some UI problems / glitches + overall poor performance
cf. following videos:
Screen.Recording.2022-09-12.at.13.59.54.mov
Screen.Recording.2022-09-12.at.14.27.12.mov
Screen.Recording.2022-09-12.at.14.28.07.mov
In comparaison, here is how it behaves on the current prod version (only the countdown is re-rendered):
Screen.Recording.2022-09-12.at.14.40.32.mov
This is mainly due to how you broke down the component hierarchy of this new provider / rates list. And also due to the fact that you handle the countdown at the root form component (Swap2/Form/index.jsx
)
Here is an example of how it could be broken down:
You would only handle the countdown logic in the red component, hence only this component would be re-rendered every second, and not the whole form (it could be broken down even more with just the countdown being it's own atomic component, like it is done today).
For the countdown UI, instead of using a new png image for the timer, you could use the already existing AnimatedCountdown
component that is currently used in prod.
d812e8e
to
0d96ca2
Compare
Fixed |
This code is not used anymore
add missing type for function props
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.
Looks good overall. Major issues from previous review are fixed.
PS: I had to fix some dead code, typing and styling issues. It looks like your environment and / or tooling might not be properly setup
It looks like the timer is still causing a warning in the dev tools. Not sure if its an issue or not. Can you check? @sarneijim @chabroA Screen.Recording.2022-09-14.at.09.44.51.mov |
Hi. Did you mentioned correct "alex"?π @ggilchrist-ledger |
Oops no I didn't! Sorry @alex-ledger |
Can you fix the wrapping of the text here so it occupies the full space (like the designs)? @sarneijim |
b4043d4
to
b9dff69
Compare
} | ||
}, | ||
"filter": { | ||
"all": "All", |
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 this viewable as part of this PR or ignore until the DEX/filter PR?
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.
yes, I am applying here the changes too advance the translations (avoid translation dependency in the next pr)
@sarneijim Please investigate the quotes refetching before the timer has expired: Screen.Recording.2022-09-15.at.17.48.21.mov |
The above has been ok'ed as acceptable for now by Xavier |
π Description
Improving swap page for the user to easily compare options
β Context
β Checklist
πΈ Demo
π Expectations to reach
Improving swap page for the user to easily compare options
Pull Requests must pass the CI and be internally validated in order to be merged.