JuliaGizmos / Escher.jl

Composable Web UIs in Julia
https://juliagizmos.github.io/Escher.jl
Other
335 stars 63 forks source link

Fixed follow-up bug to watch! and trigger! #188

Closed izaid closed 7 years ago

izaid commented 7 years ago

This is a further fix for https://github.com/shashi/Escher.jl/pull/186. That PR fixed watch! and trigger! for most widgets, but apparently not all as I found out. This PR fixes it for other widgets like checkbox.

I've also added a new example -- checkboxes.jl -- that demonstrate using a group of checkboxes, all of which are watched and triggered.

@shashi Check out checkboxes.jl. If one wants a group of triggerable and watched checkboxes, the idiom is currently trigger!(s, watch!(s, :name, checkbox(...))). Should we make a function that combines trigger! and watch!, or just leave it like this?

shashi commented 7 years ago

Can you split the checkboxes example into a separate commit?

This looks good. Thanks!

izaid commented 7 years ago

Sure. What do you think about combining watch! and trigger! for things like the checkboxes? It currently feels a bit verbose, but I'm not sure a good way to do it.

izaid commented 7 years ago

@shashi Done, the checkboxes example is now in it's own commit.

shashi commented 7 years ago

Combining trigger! and watch! makes sense for this example. Let's do that in a different PR though. I think the name should be listen! ? Too many names.

izaid commented 7 years ago

Cool, I'll do another PR after we merge this.

shashi commented 7 years ago

That's not what I meant, the example is still in the fix commit :)

I meant something like

git reset HEAD~~~ # reset the 3 commits
git add -p
# only select changes that goes into the fix
git commit
git add examples/checkboxes.jl
git commit -m "checkboxes example"
git push -f yourfork HEAD
izaid commented 7 years ago

Just entered all those commands. How's that? If it's not okay, I'll close and reopen. Just let me know.

shashi commented 7 years ago

It looks like you left out the first git commit, or that you left out selection process in git add -p

as you can see, there's still only one commit.

There's no need to have closed this PR though

izaid commented 7 years ago

So git add -p didn't ask me about checkboxes.jl, maybe because it's a new file?

If you want them separate, I'll simply close and make two PRs. That's my level of git. :)

shashi commented 7 years ago

Yeah, git add -p will not. That's kinda the point, because I don't want that file in the first commit :)

for the second commit you can add that file with git add examples/checkboxes.jl

izaid commented 7 years ago

When I do git add examples/checkboxes.jl, I get "your branch is up to date". Mind if I just close and reopen two PRs? I'd rather do that then mess around with git.

izaid commented 7 years ago

Or just merge this as is, whichever you prefer.

izaid commented 7 years ago

Cheers, thanks!