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

refactor: Better align REPL examples for comparison #1164

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Jul 3, 2024

We have a couple examples that beg comparison (counter vs counter-htm, todo-list vs todo-list-signals) but have implemented them rather differently so that the comparison mostly falls flat -- it's very much an apples-to-oranges comparison.

While that's not inherently a problem, I think it's better to more closely align these examples so that we can better show newer users the direct translation of some (very basic) concepts of these APIs we offer.

I might go pick up a list of other common examples that other framework show, might be nice to flesh this list out further.

I also ran all the examples through Prettier, some were a bit inconsistent

@rschristian rschristian merged commit ccab39c into master Jul 3, 2024
5 checks passed
@rschristian rschristian deleted the refactor/repl-examples branch July 3, 2024 20:02
Comment on lines +31 to +56
return (
<form onSubmit={addTodo}>
<input type="text" value={newItem.value} onInput={onInput} />
<button onClick={addTodo}>Add</button>
<ul>
{todos.value.map((todo, index) => (
<li>
<label>
<input
type="checkbox"
checked={todo.completed}
onInput={() => {
todo.completed = !todo.completed;
todos.value = [...todos.value];
}}
/>
{todo.completed ? <s>{todo.text}</s> : todo.text}
</label>{' '}
<button onClick={() => removeTodo(index)}>❌</button>
</li>
))}
</ul>
<p>Completed count: {completedCount.value}</p>
</form>
);
}
Copy link

Choose a reason for hiding this comment

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

The updated example doesn't work. Try removing a todo.

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems to work just fine for me? What issue are you seeing?

Copy link

Choose a reason for hiding this comment

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

The remove button doesn't actually remove the todo item, it only empties the text. The previous example works just fine.

preactjs

Copy link
Member Author

Choose a reason for hiding this comment

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

Strange, I can't reproduce. Try refreshing whilst clearing the cache, you might have something left over? Else, I'll look into it when I can.

Peek.2024-08-31.23-41.mp4

Copy link

@ushuz ushuz Sep 1, 2024

Choose a reason for hiding this comment

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

Refreshing doesn't help in my case. My browser is Chrome 128.0.6613.114 on Windows 10.

I made a longer recording, and based on my observation, it seems that clicking the remove button indeed removed the todo item, but it also triggered the form submission and appended an empty todo item:

preactjs

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yep, forgot type="button" as per usual.

Good catch!

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.

3 participants