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

NewsSite-Next workload is generating keys for <li> element on the fly #422

Open
lingyuncai opened this issue Aug 29, 2024 · 3 comments
Open

Comments

@lingyuncai
Copy link

Since the data doesn't contain an id field for each item, the workload is now generating keys for each <li> element dynamically while rendering, using uuidv4().

{content.map((item) =>
<li key={uuidv4()} className={styles["article-list-item"]}>
{item.url && !item.title
? <a href={item.url}>
<ArticleText text={item.content} />
</a>
: <ArticleText text={item.content} />
}
</li>
)}

This will cause keys to never match up between renders, leading to these elements being recreated every time, which defeats the purpose of keys and slows the performance [1].

Besides, this implementation seems inconsistent with that of NewsSite-Nuxt, which uses item.id (which is undefined) as the key binding directly.

<li v-for="item in content" :key="item.id" :class="styles['article-list-item']">
<a v-if="item.url && !item.title" :href="item.url">
<ArticleText :text="item.content" />
</a>
<ArticleText v-else :text="item.content" />
</li>

[1] https://react.dev/learn/rendering-lists#rules-of-keys

@lingyuncai
Copy link
Author

PR to address this issue: #423

@flashdesignory
Copy link
Contributor

@lingyuncai - thanks for the issue and your solution.
I opened a separate pr, where I added the unique ids to the actual data files.
In that way we don't even need to generate them at any point in the app.

#426

@lingyuncai
Copy link
Author

Hi, @flashdesignory

Thanks for your new PRs, they indeed solve this issue in a more clean way and it looks great!

Once these PRs are merged, do you know which version of Speedometer will include these changes?

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

No branches or pull requests

2 participants