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

[dashboard] Support multiple "Recent" projects with the same title #4312

Merged
merged 1 commit into from
May 27, 2021

Conversation

jankeromnes
Copy link
Contributor

@jankeromnes jankeromnes commented May 27, 2021

Fixes this:

Screenshot 2021-05-26 at 17 18 35

Explanation

I believe using the same key on multiple elements in a list breaks React. This PR attempts to make the key unique.

How to test

Expected:

  • No "jankeromnes/gitpod-staging-prebuilds" entry in "Examples"
  • Switching multiple times between "Recent" and "Examples" should not create many "jankeromnes/gitpod-staging-prebuilds" entries

@@ -35,7 +35,7 @@ export function StartWorkspaceModal(p: StartWorkspaceModalProps) {
useEffect(() => setSelection(computeSelection()), [p.recent, p.selected]);

const list = (selection === 'Recent' ? p.recent : p.examples).map(e =>
<a key={e.title} href={e.startUrl} className="rounded-xl group hover:bg-gray-100 dark:hover:bg-gray-800 flex p-4 my-1">
<a key={e.startUrl} href={e.startUrl} className="rounded-xl group hover:bg-gray-100 dark:hover:bg-gray-800 flex p-4 my-1">
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 there is no guarantee for startUrl to be unique.
In such cases it's easy to add the index of element provided by map, e.g.

map((e, index) => { 
    ... key={`element-${index}-` + e.startUrl}
})

Copy link
Member

Choose a reason for hiding this comment

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

Oh, actually looking at the effect and L.37, it'd make sense to include selection into the key as well. Sorry for missing that, @jankeromnes.

BTW, could you please make https://gitlab.com/jankeromnes/gitpod-staging-prebuilds public to make it easier to test.

Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

It works as advertised! Thanks @jankeromnes

Just a note on #4312 (comment), I've no idea how to reproduce it actually. This is good so far 👍🏻

@AlexTugarev AlexTugarev merged commit 1a1b198 into main May 27, 2021
@AlexTugarev AlexTugarev deleted the jx/support-multiple-titles branch May 27, 2021 08:00
@gtsiolis
Copy link
Contributor

gtsiolis commented May 27, 2021

Thanks for fixing this @jankeromnes! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants