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

Remove use of ChildrenRenderer from nested_list #3436

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

futursolo
Copy link
Member

Description

See: #3262

This pull request removes use of ChildrenRenderer from examples.
This further discourages manipulating Children / ChildrenRenderer which would cause keying issues.

Checklist

  • I have reviewed my own code
  • [N/A] I have added tests

@futursolo futursolo added the A-examples Area: The examples label Oct 2, 2023
github-actions[bot]
github-actions bot previously approved these changes Oct 2, 2023
@github-actions
Copy link

github-actions bot commented Oct 2, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 102.827 102.827 0 0.000%
boids 175.606 175.606 0 0.000%
communication_child_to_parent 95.303 95.303 0 0.000%
communication_grandchild_with_grandparent 109.053 109.053 0 0.000%
communication_grandparent_to_grandchild 105.724 105.724 0 0.000%
communication_parent_to_child 92.792 92.792 0 0.000%
contexts 113.453 113.453 0 0.000%
counter 89.198 89.198 0 0.000%
counter_functional 89.934 89.934 0 0.000%
dyn_create_destroy_apps 92.317 92.317 0 0.000%
file_upload 103.517 103.517 0 0.000%
function_memory_game 174.576 174.576 0 0.000%
function_router 352.473 352.473 0 0.000%
function_todomvc 163.438 163.438 0 0.000%
futures 227.445 227.445 0 0.000%
game_of_life 112.166 112.166 0 0.000%
immutable 188.781 188.781 0 0.000%
inner_html 85.980 85.980 0 0.000%
js_callback 113.455 113.455 0 0.000%
keyed_list 201.204 201.204 0 0.000%
mount_point 89.187 89.187 0 0.000%
nested_list 114.608 115.758 +1.149 +1.003%
node_refs 96.290 96.290 0 0.000%
password_strength 1721.062 1721.062 0 0.000%
portals 98.363 98.363 0 0.000%
router 318.447 318.447 0 0.000%
simple_ssr 144.252 144.252 0 0.000%
ssr_router 390.238 390.238 0 0.000%
suspense 119.023 119.023 0 0.000%
timer 91.746 91.746 0 0.000%
timer_functional 100.451 100.451 0 0.000%
todomvc 143.686 143.686 0 0.000%
two_apps 89.896 89.896 0 0.000%
web_worker_fib 138.885 138.885 0 0.000%
web_worker_prime 190.365 190.365 0 0.000%
webgl 88.553 88.553 0 0.000%

⚠️ The following example has changed its size significantly:

examples master (KB) pull request (KB) diff (KB) diff (%)
nested_list 114.608 115.758 +1.149 +1.003%

@its-the-shrimp
Copy link
Contributor

But people do need an example for the {for x} syntax, don't they? I understand the concerns about unexpected performance hits because of a hidden requirement of needing to provide keys to all VList elements, but that doesn't justify removing documentation. I'd suggest to instead improve this part of the documentation.

Comment on lines +62 to +65
vec![
html_nested! {
<ListHeader text="Calling all Rusties!" {on_hover} {list_link} key="header" />
}
Copy link
Member

Choose a reason for hiding this comment

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

I would move these vec!s out into variables but it's fine if you want to merge it as-is. It's just a stylistic thing to keep Rust code out of html! . 3 levels of nested macros doesn't look that either

@futursolo futursolo merged commit 013440d into yewstack:master Oct 3, 2023
8 checks passed
@futursolo
Copy link
Member Author

futursolo commented Oct 3, 2023

I understand the concerns about unexpected performance hits because of a hidden requirement of needing to provide keys to all VList elements

Examples should also be present to promote optimal pattern. All manipulation of an layout should be fully-keyed.

The underlying reason is not because of performance.
Not requiring key requirement renders the layout to be unstable as Yew is not able to tell what element to update.
(Layout defined by html! should be keyless safe regardless of what you do. The current {for expr} does not provide this guarantee.)

I would suggest to read the entire conversation in #3262 if you wish to learn more about the rationale of this change.

Once layout manipulation is out of the way (what this pull request is for), then we are able to impose a VList at the outer most of an {for expr} to make it keyless-safe for elements adjacent to it.

Once we made {for expr} to be sound again, we can add documentation / examples about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-examples Area: The examples
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants