widgetti / solara

A Pure Python, React-style Framework for Scaling Your Jupyter and Web Apps
https://solara.dev
MIT License
1.62k stars 105 forks source link

feat: multiple file support for file_drop #562

Closed hkayabilisim closed 1 month ago

hkayabilisim commented 2 months ago

Added multiple file support to file_drop component.
Same as the original component, directories are simply ignored. Multiple file support was actually embedded in the underlying VUE scripts. I've done minor changes.

Compatibility: on_file callback function now accepts List[FileInfo] as opposed to single FileInfo object. This may bring some problems for existing codes. As a remedy, if the number of files is just one, we can return a single FileInfo. If not, we return list. I leave it up to you to decide.

This is related to: https://github.com/widgetti/solara/issues/260

maartenbreddels commented 1 month ago

Looking good! A few issues

Looking forward to see this green in CI, and get this merged 🥳

hkayabilisim commented 1 month ago
hkayabilisim commented 1 month ago

Hi @maartenbreddels

There is only one error I couldn't resolve. Help needed!

image
maartenbreddels commented 1 month ago

Hi Huseyin,

we had quite a long discussion internally on this PR. For ToggleButton https://github.com/widgetti/solara/blob/master/solara/components/togglebuttons.py we had two different components

This is because otherwise the typing becomes quite complex (as you can see now for FileDrop), and it only works for literals. So if you want to take advantage of the typing you need to do:

if multiple:
  FileDrop(..., multiple=True)
else:
  FileDrop(..., multiple=False)

Which is a bit odd. For this reason, it might be better to have FileDrop and FileDropMultiple (they can call the same internal component btw, it's just the public API)

However, also that would be inconsistent in the naming, so either solution is not perfect.

Two questions:

  1. What do you think about the typing and one vs two components
  2. If you also prefer FileDropMultiple, are you willing to do the changes? If not, we're happy to help.

Let us know, and thanks you for your help on this.

Regards,

Maarten

hkayabilisim commented 1 month ago

Hi Maarten,

Getting rid of some complexity by creating two variants (FileDrop and FileDropMultiple) seems a good idea.

I've just created FileDropMultiple and commited to the branch and tested little bit (https://github.com/widgetti/solara/pull/562/commits/fa6cd2367b445d674e4d89ccc827f386d60bf7ac). It seems ok, and CI checks are green. Yet, you should revise my implementation in case if I missed some points.

Sincerely Huseyin

maartenbreddels commented 1 month ago

Hi Huseyin,

whow, many thank for doing all this work! CI looks green (ignore that failing one, that's not related to what you do) Will review after the weekend probably. Have a good weekend.

Regards,

Maarten

hkayabilisim commented 1 month ago

No problem Maarten! I've added _FileDrop to prevent code repetitions. I hope this is what you have in your mind!

iisakkirotko commented 1 month ago

Hi @hkayabilisim! Just wanted to let you know that the CI failure is still unrelated, it should be fixed by https://github.com/widgetti/solara/pull/574. We'll try to get this PR in soon™️ :)

maartenbreddels commented 1 month ago

This branch had some flakiness that we wanted to attack (see the last commit) before merging it. Thanks for your patience!