-
Notifications
You must be signed in to change notification settings - Fork 5.2k
feat: edit custom network flow #25272
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
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. |
94737c1
to
5196425
Compare
/> | ||
</> | ||
); | ||
} | ||
|
||
AddNetworkModal.propTypes = { | ||
isNewNetworkFlow: PropTypes.bool, | ||
addNewNetwork: PropTypes.bool, |
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.
We need comments for each of these props.
ui/components/multichain/network-list-item-menu/network-list-item-menu.js
Outdated
Show resolved
Hide resolved
ui/components/multichain/network-list-item-menu/network-list-item-menu.js
Outdated
Show resolved
Hide resolved
ui/components/multichain/network-list-item-menu/network-list-item-menu.js
Outdated
Show resolved
Hide resolved
An annoying thing is how Screen.Recording.2024-06-13.at.3.28.13.PM.mov |
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.
approving with a few suggestions
fd5644c
to
3f38e5c
Compare
ab2b46c
to
3e2d41e
Compare
19ffda0
to
6d4b3ac
Compare
ui/components/multichain/network-list-item-menu/network-list-item-menu.js
Outdated
Show resolved
Hide resolved
6d4b3ac
to
9b507a0
Compare
9b507a0
to
667e58a
Compare
{editedNetwork ? ( | ||
<ActionableMessage | ||
type="success" | ||
className="home__new-tokens-imported-notification" |
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 class name correct? Should it be new network instead of new tokens?
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.
It's because I'm using an existing CSS class; I don't want to create a new class.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #25272 +/- ##
=========================================
Coverage 64.93% 64.93%
=========================================
Files 1385 1386 +1
Lines 54870 55010 +140
Branches 14408 14455 +47
=========================================
+ Hits 35625 35717 +92
- Misses 19245 19293 +48 ☔ View full report in Codecov by Sentry. |
Builds ready [667e58a]
Page Load Metrics (49 ± 5 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
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.
ran the branch and looked at the code, LGTM
Description
Edit network flow from the modal added
Related issues
Fixes:
Manual testing steps
yarn && ENABLE_NETWORK_UI_REDESIGN=1 yarn start
Screenshots/Recordings
Before
before.mov
After
after.mov
Pre-merge author checklist
Pre-merge reviewer checklist