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

xilem_masonry: Add filter ("All", "Active", "Completed") to todomvc example #265

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Philipp-M
Copy link
Contributor

@Philipp-M Philipp-M commented May 5, 2024

This adds the filters of TodoMVC via checkboxes currently (as we currently don't have better widgets for that...).

It also revealed (at least) 2 bugs, one being fixed by #264 and another I (or someone else) needs to investigate (difficult to describe just add a few todos, check them, and click all the filters, it should be easy to reproduce).

@Philipp-M Philipp-M force-pushed the xilem_masonry-todomvc-add-filter branch from f4f11bb to 1767ca5 Compare May 5, 2024 16:49
@PoignardAzur
Copy link
Contributor

Nicely done, but as you point out, this PR makes these other bugs very visible. I'd rather not merge it until they are fixed, since this is our showcase. ^^

@Philipp-M
Copy link
Contributor Author

Sure, I also think this should only be merged, when it doesn't produce obvious bugs at this point :). But I think it's probably good for debugging these bugs.

@Philipp-M Philipp-M force-pushed the xilem_masonry-todomvc-add-filter branch from 1767ca5 to c95fcd5 Compare May 5, 2024 18:13
@Philipp-M
Copy link
Contributor Author

(Now this weird issue can be debugged, since #264 is merged)

@bram209
Copy link
Contributor

bram209 commented May 6, 2024

I ran in (I think) the same issue that you are describing, put some reproduction steps here: https://xi.zulipchat.com/#narrow/stream/354396-xilem/topic/TodoMVC.20example/near/437314215

github-merge-queue bot pushed a commit that referenced this pull request May 6, 2024
…has changed (#278)

Fixes the issue described in #265.

I *think* this is the proper fix, it seems to be similar as in previous
xilem (where I think that pass is called `ViewContextChanged`).
@Philipp-M Philipp-M force-pushed the xilem_masonry-todomvc-add-filter branch from c95fcd5 to c0c1b73 Compare May 6, 2024 20:02
@Philipp-M
Copy link
Contributor Author

Since #278 is merged, I've tested this again, it works now as expected

@jaredoconnell
Copy link
Contributor

I tested this. I think this filter should be placed before the list:
image
This makes it so that it doesn't move around when you're using it.

I do find that it's in need of polish. You're using multi-select checkboxes as radio buttons. If we're going with multi-select, you technically don't want an All option, and instead check both options.
I don't know what level of polish we're going for this week.

@xStrom
Copy link
Member

xStrom commented May 6, 2024

I think this filter should be placed before the list:
This makes it so that it doesn't move around when you're using it.

For what it's worth, it also moves around in the examples on todomvc.com.

@Philipp-M
Copy link
Contributor Author

Yes checkboxes are not ideal indeed, these should be exchanged with the right widgets, when they're implemented.

I'm not sure how much we're striving towards the original TodoMVC.

@flosse flosse added the masonry Issues relating to the Masonry widget layer label Aug 13, 2024
@PoignardAzur
Copy link
Contributor

I've been looking through our PR backlog, and I'd be interested in merging this, if everything works correctly after a rebase.

I'm fine having checkboxes masquerade as combo buttons for now, as long as we track it in an issue. We can add a big ugly "TODO - replace with combo buttons" comment to the example.

@Philipp-M Philipp-M force-pushed the xilem_masonry-todomvc-add-filter branch from c0c1b73 to bc556fb Compare August 16, 2024 12:04
@Philipp-M
Copy link
Contributor Author

I've rebased it (+ added comment), and well it revealed yet another new bug^^ (this example seems to be a quite good test as well)

To reproduce:
Press "Active" and then
"Buy milk"

@DJMcNab
Copy link
Member

DJMcNab commented Aug 16, 2024

Hmm, so the behaviour we see is that the checkbox "thinks" it is clicked, but without sending an action.

That's very strange

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
masonry Issues relating to the Masonry widget layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants