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

Make sure the breadcrumb has unique keys #458

Merged
merged 1 commit into from
May 22, 2024

Conversation

jelly
Copy link
Member

@jelly jelly commented May 16, 2024

A path can have the same directory so make sure the keys are actually unique.

@@ -105,12 +105,12 @@ export function FilesBreadcrumbs({ path, showHidden, setShowHidden }: { path: st
/>}
{!editMode && fullPath.map((dir, i) => {
return (
<React.Fragment key={dir || "/"}>
<React.Fragment key={`${dir}-${i}`}>
Copy link
Member

Choose a reason for hiding this comment

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

i must not be part of a key. The point of keys are to uniquely identify the content/identity of an entry independently from its position (otherwise React wouldn't need keys in the first place), to know which rows have to be re-rendered. I suggest to build up the key from the full path during the iteration, like "usr", "usr/share", "usr/share/doc", etc. An external accumulator variable should work, or a slice of path.

It may work fine in this specific case, but it's a very dangerous practice, so please don't learn it 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, it's a bit ugly to have a var defined outside but I don't see a nicer way to do it.

Copy link
Member Author

Choose a reason for hiding this comment

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

And reworked to use fullPath.

@jelly jelly force-pushed the duplicate-key-breadcrumb branch from 085431b to 8ebe825 Compare May 21, 2024 16:43
@jelly jelly requested a review from martinpitt May 21, 2024 16:43
@jelly jelly force-pushed the duplicate-key-breadcrumb branch from 8ebe825 to b6c8a8e Compare May 21, 2024 16:47
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks! fullPath.slice() is a nice solution.

<Button
isDisabled={i === path.length - 1}
icon={i === 0 ? <HddIcon /> : null}
variant="link" onClick={() => { navigate(i + 1) }}
key={dir} className="breadcrumb-button"
key={`${dir}-${i}`} className="breadcrumb-button"
Copy link
Member

Choose a reason for hiding this comment

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

This still uses i. But why does hthis need a key at all? It's not a dynamic list inside the fragment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, dropped it.

A path can have the same directory so make sure the keys are actually
unique.
@jelly jelly force-pushed the duplicate-key-breadcrumb branch from b6c8a8e to dd79521 Compare May 22, 2024 08:17
@jelly jelly requested a review from martinpitt May 22, 2024 08:22
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Cheers!

@jelly jelly merged commit 92214e8 into cockpit-project:main May 22, 2024
10 of 12 checks passed
@jelly jelly deleted the duplicate-key-breadcrumb branch May 22, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants