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.

Sign up for GitHub

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

Jump to bottom

Fix throwing SharedToggleItem on item without label #3486

Closed
wants to merge 2 commits into from
Closed

Fix throwing SharedToggleItem on item without label #3486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7886ead
Fix throwing SharedToggleItem on item without label
jagodarybacka Jun 16, 2023
da343f7
Allow `SharedToggleItem` label to be `undefined`
jagodarybacka Jun 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion background/lib/utils/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ export function getEthereumNetwork(): EVMNetwork {
}

export function isProbablyEVMAddress(str: string): boolean {
if (normalizeHexAddress(str).startsWith("0x") && str.length === 42) {
if (str.length === 42 && normalizeHexAddress(str).startsWith("0x")) {
return true
}
return false
Expand Down
6 changes: 4 additions & 2 deletions ui/components/Shared/SharedToggleItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import SharedToggleButton from "./SharedToggleButton"
export const STARS_GREY_URL = "./images/stars_grey.svg"

type SharedToggleItemProps = {
label: string
label?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Non-blocking comment
I wonder if this is the correct place for the fix. 🤔 My point is that the problem is not with the SharedToggleItem component but with the undefined name value for collection. Therefore, I don't know whether we shouldn't fix this issue here. Because now the changes apply to the shared component what is a bit unclear is why the label can be optional. What I mean is that the thumbnailURL is already optional and with the optional label it reduces this component to a simple SharedToggleButton. Maybe we could add a comment for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tbh I'm now hesitant to even merging this fix, as I was not able to reproduce it and in theory the name should be replaced with empty string if needed while the collection is fetched from SH. Unless we are able to find a way to reproduce it 🤔
cc @michalinacienciala can you try to reproduce that problem with NFTs filters?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to reproduce it. A bit different steps than before: I reproduced it in Playwright tests when switching networks quickly (Ethereum -> Optymism -> Polygon -> Arbitrum) and then going straight away to NFTs filtering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have this test already in the codebase? if not can you create a draft PR with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Added temporary test in #3512.

thumbnailURL?: string
checked: boolean
onChange: (toggleValue: boolean) => void
Expand All @@ -25,7 +25,9 @@ export default function SharedToggleItem({
<div className="text_wrap">
<div className="thumbnail" role="img" />
<label className="label ellipsis">
{isProbablyEVMAddress(label) ? truncateAddress(label) : label}
{typeof label === "string" && isProbablyEVMAddress(label)
? truncateAddress(label)
: label ?? ""}
</label>
</div>
<SharedToggleButton onChange={onChange} value={checked} />
Expand Down