search

widgetti / ipyreact

React for ipywidgets that just works. No webpack, no npm, no hassle
BSD 3-Clause "New" or "Revised" License
104 stars 8 forks source link

Refactor Observe example #14

Closed kolibril13 closed 1 year ago

kolibril13 commented 1 year ago

I renamed some variables, changed the traitlets variable type and put the check_is_even logic in a separete function. I think this makes this example easier to understand. What do you think @paddymul?

paddymul commented 1 year ago

I like it. Maybe name this EvenOddWidget?

Generally I like examples to be small and simple for each facet of functionality. If I was building with this and I ran into a bug, I would scan for examples to find one that was doing something close to what I wanted, then change that example closer to my usecase, or try to spot what I was doing wrong.

Examples that require a lot of baggage dependencies are a slower to understand. This is good as is.

As a side note, I know that with traitlets you get the change event, unless I'm specifically interested in the delta, I choose to just access the value on the instance self.count vs reaching into the change event change['new']. Not sure about the recommended practice.

kolibril13 commented 1 year ago

Generally I like examples to be small and simple for each facet of functionality

I absolutely share this philosophy!

What I really like is to have minimal boilerplate examples that showcase exactly one feature, which I can then use to build my scripts for my usecases.

Regarding the traitlets: I think self.count looks more familiar, and is easier to understand, so I'd go with that.

kolibril13 commented 1 year ago

check_is_even should not return a string, but a boolean

this is adjusted now!

maartenbreddels commented 1 year ago
image

oops, code suggestions for notebooks are not ideal ;), sorry for breaking it, I think you need to manually edit the json file

paddymul commented 1 year ago

What is the suggested way to do PRs on notebooks. I have heard of a bunch of projects around this. What are your thoughts?

maartenbreddels commented 1 year ago

I prefer python files in git :)

kolibril13 commented 1 year ago

Yes, some invalid notebook syntax was introduced by 14719dc, but I manually fixed that, now the notebook div should look good again.

image

Tip: You can enable "Rich Jupyter Notebook Diffs" in Github. It's still beta, but already works quite well:

image
kolibril13 commented 1 year ago

@maartenbreddels : This is now ready to be merged.