-
Notifications
You must be signed in to change notification settings - Fork 146
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: multiple file support for file_drop #562
feat: multiple file support for file_drop #562
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution @hkayabilisim!
We would like to keep this component backward compatible. We could accomplish this by adding an argument multiple
with a default value of False
and have on_file also work with a single file (see this for an example of the typing, note the argument optional: Literal[False]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing ';' at the end of the "padding:" line was causing CSS problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes required for backward compatibility:
- multiple property is added to FileDrop
- Two different on_file callback signature used: FileInfo and List[FileInfo]
- FileDrop is overloaded to match two different on_file signatures.
- FileDrop apidoc is updated
- FileDrop example is updated
- bullet list in VUE template is removed
- isFile property is always set to True (this was a hidden bug)
- app/scatter.py is reverted to original version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments of @mariobuikhuizen are handled in 7d44858ec65c103c299d9757c4ef169e5f5b9f9b
Looking good!
Looking forward to see this green in CI, and get this merged 🥳 |
7d44858
to
0c33e33
Compare
solara/website/pages/documentation/components/input/file_drop.py
Outdated
Show resolved
Hide resolved
solara/website/pages/documentation/components/input/file_drop.py
Outdated
Show resolved
Hide resolved
Hi Huseyin, we had quite a long discussion internally on this PR.
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:
Which is a bit odd. For this reason, it might be better to have However, also that would be inconsistent in the naming, so either solution is not perfect. Two questions:
Let us know, and thanks you for your help on this. Regards, Maarten |
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 (fa6cd23). It seems ok, and CI checks are green. Yet, you should revise my implementation in case if I missed some points. Sincerely |
Hi Huseyin, whow, many thank for doing all this work! Regards, Maarten |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work.
Let me know if you have the energy to do these last changes. I know we're asking a lot from you, so we are happy to do the last mile.
No problem Maarten! I've added _FileDrop to prevent code repetitions. I hope this is what you have in your mind! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may not fit 100% to the code styling requirements but I think my last commit e5e6f40 solves most of the highlighted issues.
One of the checks keeps failing but I couldn't understand why.
e5e6f40
to
4ca3871
Compare
Hi @hkayabilisim! Just wanted to let you know that the CI failure is still unrelated, it should be fixed by #574. We'll try to get this PR in soon™️ :) |
fb71402
to
2589372
Compare
Co-authored-by: Iisakki Rotko <iisakki.rotko@gmail.com>
"Navigation to X is interrupted to another navigation to Y" In the logs we see: ``` FAILED tests/integration/ssg_test.py::test_ssg[starlette-chromium] - playwright._impl._errors.Error: Navigation to "http://localhost:18768/" is interrupted by another navigation to "http://localhost:18768/" FAILED tests/integration/starlette_test.py::test_starlette_mount[starlette-chromium] - playwright._impl._errors.Error: Navigation to "http://localhost:18770/solara_mount/" is interrupted by another navigation to "http://localhost:18768/" ERROR tests/integration/widget_test.py::test_widget_button_solara[starlette-chromium] - playwright._impl._errors.Error: Navigation to "http://localhost:18768/?id=4004e29b-a9e6-4b1e-9652-7b1c61ef86cd" is interrupted by another navigation to "http://localhost:18770/solara_mount/" ERROR tests/integration/widget_test.py::test_solara_button_all[starlette-solara-chromium] - playwright._impl._errors.Error: Navigation to "http://localhost:18768/?id=c8f0b1fb-abdb-4969-b340-1156d2699783" is interrupted by another navigation to "http://localhost:18768/?id=4004e29b-a9e6-4b1e-9652-7b1c61ef86cd" ERROR tests/integration/widget_test.py::test_slider_all[starlette-solara-chromium] - playwright._impl._errors.Error: Navigation to "http://localhost:18768/?id=680f1fc7-5a19-4e1c-a382-088f2ec96b78" is interrupted by another navigation to "http://localhost:18768/?id=c8f0b1fb-abdb-4969-b340-1156d2699783" ```
2589372
to
5075405
Compare
This branch had some flakiness that we wanted to attack (see the last commit) before merging it. Thanks for your patience! |
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: #260