Skip to content
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

Fix: prevent key prop warning in EthHashInfo #2351

Merged
merged 3 commits into from
Aug 7, 2023
Merged

Fix: prevent key prop warning in EthHashInfo #2351

merged 3 commits into from
Aug 7, 2023

Conversation

katspaugh
Copy link
Member

This PR removes the pesky React warning about a "key" prop in the console.

I fixed it by copying EthHashInfo from safe-react-components to this repo because the problem stems from the safe-react-components's build not working well with this app's build system.

Also updated dependencies along the way.

Fix: prevent key prop warning in EthHashInfo

Fix: prevent key prop warning in EthHashInfo
@github-actions
Copy link

github-actions bot commented Aug 4, 2023

Branch preview

✅ Deploy successful!

https://eth_hash_info--walletweb.review-wallet-web.5afe.dev

@github-actions
Copy link

github-actions bot commented Aug 4, 2023

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

{showAvatar && (
<AvatarContainer size={avatarSize}>
{!fallbackToIdenticon && customAvatar ? (
<img src={customAvatar} alt={address} onError={onError} width={avatarSize} height={avatarSize} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can use ImageFallback here.

Copy link
Member

@usame-algan usame-algan Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to adjust ImageFallback to support a custom component as src. Maybe easier to leave it as it is

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ImageFallback has a fallbackComponent prop.

)
}

const Container = styled('div')({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only being used here. Can we use a div with a class or Box instead?

},
}))

const AddressContainer = styled('div')({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar here, its only used once so we could change it to a div with a class or Mui Box

Comment on lines +14 to +21
<IconButton
className={className}
target="_blank"
rel="noreferrer"
href={href}
size="small"
sx={{ color: 'inherit' }}
>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a stopPropagation call onClick here. Is it not needed anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where was it exactly? I can't find it in the change set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was part of safe-react-components

size="small"
sx={{ color: 'inherit' }}
>
<SvgIcon component={icon} inheritViewBox fontSize="small" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The custom Icon component had some css set on the SvgIcon. Is it still covered after porting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks the same to me in the end result, but again, I'm not sure which original code you're referring to. In safe-react-components?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I was comparing it to what we have in safe-react-components.

Copy link
Member Author

@katspaugh katspaugh Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, stopPropagation was actually necessary to prevent NFT previews from opening on the NFT page (5afe/safe-react-components#220).
I'll restore it in another PR.

@@ -100,7 +100,7 @@ describe('ApprovalEditorForm', () => {

// Change value and save
const amountInput = result.container.querySelector('input[name="approvals.0"]') as HTMLInputElement
const saveButton = result.getByRole('button')
const saveButton = result.getByTitle('Save')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@katspaugh
Copy link
Member Author

I'm going to merge this as there are no visual changes here.

@katspaugh katspaugh merged commit 8826af3 into dev Aug 7, 2023
6 of 7 checks passed
@katspaugh katspaugh deleted the eth-hash-info branch August 7, 2023 11:38
@github-actions github-actions bot locked and limited conversation to collaborators Aug 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants