Merged
Conversation
Contributor
|
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. |
Collaborator
Builds ready [e3cde27]
Page Load Metrics (126 ± 149 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
FrederikBolding
previously approved these changes
Jun 7, 2024
FrederikBolding
previously approved these changes
Jun 11, 2024
georgewrmarshall
previously approved these changes
Jun 11, 2024
Contributor
There was a problem hiding this comment.
LGTM! UI is looking great 💯 a few non-critical things to remember that we should probably add some linting for
- We should try to avoid inline styles in favor of custom CSS classnames
- It would be great if we can replace deprecated components in files that are updated
ui/components/multichain/address-list-item/address-list-item.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+170
to
+175
| await driver.clickElement('.settings-page__address-book-button'); | ||
|
|
||
| // it checks if account is deleted | ||
| const exists = await driver.isElementPresent(contact); | ||
| const exists = await driver.isElementPresent( | ||
| By.css('.address-list-item__label'), | ||
| ); |
Contributor
There was a problem hiding this comment.
non-blocking: CSS selectors are generally discouraged in favor of data-test-ids or tags and text
https://github.com/MetaMask/contributor-docs/blob/main/docs/e2e/extension-e2e-guidelines.md#guidelines
Contributor
Author
There was a problem hiding this comment.
findElement was hanging and so I had to use isElementPresent and the By class, I don't see any static method that lets me select with data-test-ids or text.
|
|
||
| await driver.pasteIntoField( | ||
| 'input[placeholder="Enter public address (0x) or ENS name"]', | ||
| '.ens-input__wrapper__input', |
Contributor
There was a problem hiding this comment.
non-blocking: CSS classname selectors are generally discouraged
Contributor
Author
There was a problem hiding this comment.
hmm for this one, the selector wasn't working for some reason, had to use this method.
Comment on lines
+132
to
+136
| iconProps={{ | ||
| color: IconColor.infoInverse, | ||
| style: { width: '12px', height: '12px' }, | ||
| name: IconName.Snaps, | ||
| }} |
Contributor
There was a problem hiding this comment.
Will try to resolve the size issue with the AvatarIcon icon size in this PR #25155
| {t('deleteContact')} | ||
| </Button> | ||
| <Box className="settings-page__address-book-button"> | ||
| <Button |
Contributor
There was a problem hiding this comment.
non-blocking: it would help if we can replace deprecated code with component-library in files that are updated
Contributor
Author
There was a problem hiding this comment.
I'll replace once we get the BadgeIcon in, thanks!
dd74c50
Collaborator
Builds ready [eeef3d8]
Page Load Metrics (47 ± 3 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
georgewrmarshall
approved these changes
Jun 11, 2024
Collaborator
Builds ready [27ef0ac]
Page Load Metrics (48 ± 5 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
FrederikBolding
approved these changes
Jun 11, 2024
Collaborator
Builds ready [d9d1db4]
Page Load Metrics (619 ± 459 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
jpuri
reviewed
Jun 12, 2024
| className="ens-input__wrapper__action-icon-button" | ||
| onClick={() => { | ||
| if (userInput) { | ||
| if (userInput.length > 0) { |
Contributor
There was a problem hiding this comment.
May be if (userInput?.length > 0) {
jpuri
approved these changes
Jun 12, 2024
Collaborator
Builds ready [cb4df56]
Page Load Metrics (43 ± 2 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR brings the domain resolution integration up to specification.
There's a lot going on in this PR so here's a quick breakdown of what's going on:
DomainInputResolutionCellcomponent is responsible for displaying a single resolution, it can properly display both ENS and snap provided resolutions. It also has logic to detect overflowing titles so that it can replace the title component with one that has an ellipsis and a tooltip showing the entire title.AddressListItemcomponent snapshot is because of the update to the confusable component.I had to update implementation of the(These components have since been removed)AddRecipientcomponents because the update in the name resolution api would break the existing implementation. Those components will be removed at some point by the extension team.AddContactcomponent was updated to use the domain input resolution cell component.AddressListItemcomponent was updated to accommodate overflowing titles (specifically when viewing all contacts).ViewContact&EditContactcomponents were also updated to accommodate overflowing titles.Previous
AddContactscreen:317744893-1c15eebe-3a17-49df-8923-8cb84540761c.mov
New screens:
domainresolution.mov
Note: The
AddContactscreen was re-done to accommodate for snap provided resolutions and to update the designs as it seemed the screen was largely undesigned. See previous design video.