-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Show a balance for the Token To, update position of the MM fee, removes a link #20030
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
E2E test failures seem related @dan437 |
Codecov Report
@@ Coverage Diff @@
## develop #20030 +/- ##
===========================================
- Coverage 69.47% 69.47% -0.00%
===========================================
Files 988 988
Lines 37313 37323 +10
Branches 9989 9996 +7
===========================================
+ Hits 25923 25929 +6
- Misses 11390 11394 +4
|
`; |
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.
This change seems unintentional, and should likely be removed from the 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.
I've updated snapshots and this is what I got. When I run yarn jest --updateSnapshot
now, it doesn't give me anything new.
color: var(--color-primary-default); | ||
} | ||
|
||
&__quote-rate--no-link { | ||
cursor: text; | ||
color: var(--color-text-default); |
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 use SCSS colors versus the color prop on Typography or Box?
expect(getByText('Includes a 1% MetaMask fee –')).toBeInTheDocument(); | ||
expect(getByText('view all quotes')).toBeInTheDocument(); |
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.
Does this constitute a change in our fee or just moving it around?
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.
No change in the fee logic, just making it clearer that our MM fee is included in quotes already.
@dan437 Looks like merging this start failing unit tests on Difference in behavior due to dc9801a not being rebased on |
Explanation
Screenshots
Testing