search

jupyter-widgets / ipydatagrid

Fast Datagrid widget for the Jupyter Notebook and JupyterLab
BSD 3-Clause "New" or "Revised" License
579 stars 51 forks source link

fix: do not share the Expr for text_color and background_color #400

Closed maartenbreddels closed 1 year ago

maartenbreddels commented 1 year ago

Every TextRenderer shares the same Expr (by default). This means that if I create two text renderers, and change the expression on one, it also changes the expression in the other TextRenderer.e.g.


tr1 = TextRenderer()
tr2 = TextRenderer()
tr1.text_color.value = "'red'"
# now tr2.text_color.value is also red, bad UX I think

I think this should behave analogous to say a .layout widget, where they are not shared.

To be honest, this is not the original reason to create this PR. In solara, we cannot have widgets created at import time, since each user would share the same widget, which breaks ipydatagrid in solara.

I hope one of the two reasons is compelling enough :)

maartenbreddels commented 1 year ago

Originally introduced in https://github.com/bloomberg/ipydatagrid/pull/35 by @martinRenou

martinRenou commented 1 year ago

The commit I made from the Github interface has not been signed off, sorry for that! You will probably need to edit that commit (maybe you can also squash all commits into a single signed off one)

maartenbreddels commented 1 year ago

ok, squashed them, but needs you approval again :)

maartenbreddels commented 1 year ago

Looks like this collides with this https://github.com/jupyter-widgets/ipywidgets/pull/3358/files I think we should open a separate PR and fix main/master first. What do you think @martinRenou ?

martinRenou commented 1 year ago

It's odd that we did not hit that before. PRs were green a couple of weeks ago. Thanks for the pointer! I agree we should probably fix master first.