-
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
Updated 1 NetworkDisplay component #20825
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. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #20825 +/- ##
===========================================
- Coverage 68.25% 68.25% -0.00%
===========================================
Files 1006 1006
Lines 40189 40194 +5
Branches 10743 10746 +3
===========================================
+ Hits 27429 27431 +2
- Misses 12760 12763 +3
☔ View full report in Codecov by Sentry. |
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 @MaxwellDG, apologies I left a review on this closed PR with the same changes here #20787 (review)
@georgewrmarshall I've updated it to have no changes to the PickerNetwork component. I'll keep that in mind for all future issues to not touch the components in the new component library. |
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.
Excellent updates! I've left a few more suggestsions
...firm-page-container/confirm-page-container-header/confirm-page-container-header.component.js
Show resolved
Hide resolved
...firm-page-container/confirm-page-container-header/confirm-page-container-header.component.js
Outdated
Show resolved
Hide resolved
...firm-page-container/confirm-page-container-header/confirm-page-container-header.component.js
Outdated
Show resolved
Hide resolved
ui/components/app/confirm-page-container/confirm-page-container-header/index.scss
Outdated
Show resolved
Hide resolved
… in the confirmations header component
...firm-page-container/confirm-page-container-header/confirm-page-container-header.component.js
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.
LGTM! I added one small update to allow for image functionality. Thanks for your contribution @MaxwellDG
@georgewrmarshall For the life of me I can't figure out why the "ci/circleci: test-e2e-chrome-mmi" test continues to fail. Do you have any hints as to why this would be failing? Is it the data-testIds? |
Hey @MaxwellDG, I'm not sure either I'll ask one of the MMI team if they can help support |
Hey @MaxwellDG, could you update the snapshots on this PR |
2c7cc2e
to
27904b0
Compare
@georgewrmarshall Could I get some feedback on what's missing here? I could be wrong, but from looking at the vast majority of PRs, it seems like some of the e2e tests are broken. Is there something I could be doing to improve this? I'd like to move on to updating many of the other issues that require component switching but I'd like to ensure this is done correctly first. |
Hey @MaxwellDG, we are dealing with some flakey e2e tests at the moment but the team is working on a fix. Apologies I realize this is frustrating and I appreciate your patience. Your other PRs LGTM. Once develop is passing that's usually a good sign that you can update this PR and it should pass. |
Okay thanks for the update. I'll check in periodically then and we'll get it going when develop is ready. |
This is my first PR for Metamask, so apologizes if I've overlooked a couple parts.
This is fixing 1 instance of #20485. For this issue to be closed, there's several more NetworkDisplay tags to be removed. (Should I be creating a new PR each time? Or will we be doing multiple steps of approval in this single PR?)
As stated by @georgewrmarshall in #20485, there are some UI differences. I didn't write the PickerNetwork component, but I can point out 3 differences from the NetworkDisplay component:
The first letter in the coloured circle when an img is not present
Height of component is now set to 32px
Margin and paddings are slightly different
Here's photos comparing the two:
Initial:
confirm-page-container-header component-initial
Current:
In regards to the 3 file maximum, I know I went a little bit over. I believe this was unavoidable since there's also a couple snapshot file updates in order to ensure all tests continue to pass.