Switch Widget Port #132
Switch Widget Port #132
Conversation
giannissc
force-pushed
the
main
branch
2 times, most recently
from
2775987
to
4b73324
Compare
giannissc
force-pushed
the
main
branch
2 times, most recently
from
569cfe3
to
0850945
Compare
|
This was a good exercise! I think I understand a bit more the Xilem model. Thank for the comments they were really helpful :) It's ready for another review! @Philipp-M |
|
Just rebased and force pushed with the new changes @Philipp-M |
There was a problem hiding this comment.
Sorry for the delay, haven't found the time till now...
Some nit-picks, not really that important.
I think this is good to merge when it's rebased (for the new CI workflows) and the CI is fixed (I think you can just disable clippy for that line), thanks!
I found a bug orthogonal to this PR while testing this, maybe I'll look into this (dragging the knob or pressing a button in the h_stack and then moving up to the top button and releasing the cursor)
src/widget/switch.rs
Outdated
| pub struct Switch { | ||
| id_path: IdPath, | ||
| is_on: bool, | ||
| is_moved: bool, |
There was a problem hiding this comment.
(Slightly nit picky) I would name this something like is_dragging as I think it's more appropriate...
src/widget/switch.rs
Outdated
| if self.is_on != (self.knob_position.x > SWITCH_WIDTH / 2.0) { | ||
| cx.add_message(Message::new(self.id_path.clone(), ())) | ||
| } | ||
| } else if cx.is_active() { |
There was a problem hiding this comment.
(Also super nit-picky, because it shouldn't really change the behavior, as a MouseMove event should happen before)
| } else if cx.is_active() { | |
| } else if cx.is_active() && cx.is_hot() { |
There was a problem hiding this comment.
I am replying here even though I will be creating a new PR. The is_hot check I think is wrong here and the only reason it doesn't affect the behaviour is because no actual message is being send here. Had there been any message being sent to the view it would not have if the user moved past the boundaries of the widget. Good UX would be to ensure things function correctly even in light of errors from the user
There was a problem hiding this comment.
Hmm I guess it really depends. A button is not clicked when the user releases it outside the button area. On touchscreens I agree with you though, or generally when the switch is dragged, but this should never be the case here, since is_dragging will be set before, so it doesn't really matter...
|
Is this supposed to be closed? |
|
I know what happened here.. It was before I started using git extensively and I had all these into main instead of it's own branch so when synchronized my main to master I discarded my commits for the switch widget.. |
|
They are still recoverable though, GitHub keeps everything that's pushed to a PR: 6275db8 (parent commit are also available, and clickable in the web UI) |
|
That's ok it was easy enough to start fresh. But I will keep this in mind next time this happens. Thanks! |
|
I'm not sure though, whether it's possible to change the source branch (see https://stackoverflow.com/questions/42381557/changing-source-branch-for-pr-on-github) So either you'll cherry-pick all the relevant commits onto your main branch and reopen this PR (after merging, you can of course sync with upstream main again), or you have to create a new PR with a different source branch. |
|
It's not possible to reopen a PR after the branch that was used to create it is force-pushed while the PR is closed. |
|
Oh really, interesting, thanks for letting me know. |
No description provided.