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

Use keyed lists in tutorial example #2948

Merged
merged 4 commits into from Nov 8, 2022
Merged

Conversation

allan2
Copy link
Contributor

@allan2 allan2 commented Nov 3, 2022

The example should follow best practices such as using keyed lists.

The example should follow best practices such as using keyed lists.
@github-actions
Copy link

github-actions bot commented Nov 3, 2022

Visit the preview URL for this PR (updated for commit 0faea9e):

https://yew-rs--pr2948-patch-3-0f6ey24i.web.app

(expires Mon, 14 Nov 2022 17:06:39 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@hamza1311
Copy link
Member

Can you also explain what the key is and link to the keys doc page? Someone reading the tutorial may not be familair with the concept of keyed list and a random prop that the component doesn't even accept or make use of may seem weird to them

@allan2
Copy link
Contributor Author

allan2 commented Nov 3, 2022

Added!

hamza1311
hamza1311 previously approved these changes Nov 6, 2022
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

Looks good!

@hamza1311 hamza1311 enabled auto-merge (squash) November 6, 2022 13:21
Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

The lines later in

- <p>{format!("{}: {}", video.speaker, video.title)}</p>
+ <p onclick={on_video_select}>{format!("{}: {}", video.speaker, video.title)}</p>
also should have the key prop added. Other than that, it looks good.

auto-merge was automatically disabled November 7, 2022 16:57

Head branch was pushed to by a user without write access

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks for the effort 🎉

@hamza1311 hamza1311 merged commit a5f844d into yewstack:master Nov 8, 2022
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