Make widget dropdowns look more consistent + convert BaseColorPicker to functional component
#10083
Conversation
jrieke
added
security-assessment-completed
jrieke
changed the title
jrieke
changed the title
| // We force an update by changing the key of the popover after this error, | ||
| // to re-mount the UIPopover - because the error sometimes cause it to be | ||
| // unmounted. This is an unfortunate hack. | ||
| setPopoverKey(prev => prev + 1) |
There was a problem hiding this comment.
I think this does the same thing as before but I'm not sure if it always works well and if that's the most elegant solution.
There was a problem hiding this comment.
Hmm, I'm not 100% confident that this is handling the situation correctly. Have you been able to trigger the SecurityError somehow? The previous implementation used an error boundary, which also prevented the error from bubbling up to the window. Error boundaries are not supported by functional components. This will just react to an error in the window, but this error might cause other issues in the application itself.
There was a problem hiding this comment.
Ok I just deployed this on Cloud with this part removed and I can't see that error anymore (also verified that it's indeed using this version with the removed code). Maybe that went away because the app and iframe are nowadays running on the same domain in Cloud? It might still be a problem in other deployment setups though. Anyway, I'll remove this code for now and I think we can just wait and see if someone files a GitHub issue for this, or wdyt?
There was a problem hiding this comment.
I'm a bit worried that it might crash in SiS or notebooks. Can you test if it also works correctly of you embed the cloud app on a different domain (e.g. notion)? This might be a potential patch/workaround to fix the underlying issue: uiwjs/react-color#81 (comment)
There was a problem hiding this comment.
Good call, it does happen in an embedded app and results in this exception being shown:
There was a problem hiding this comment.
Applied the workaround (had to modify a few things for TypeScript to be happy), and it seems to work well. Tested this on Cloud and in an embedded app and I don't see errors anymore.
There was a problem hiding this comment.
I think it might be quite simple to test if this works correctly via our iframed app testing in hostframe_app. Just add a color picker in the app and a test that verifies that the popup can be closed without running into the issue.
There was a problem hiding this comment.
Oh that's a sweet idea. Added a small test.
jrieke
changed the title
BaseColorPicker to functional component
jrieke
changed the title
BaseColorPicker to functional componentBaseColorPicker to functional component
| // We force an update by changing the key of the popover after this error, | ||
| // to re-mount the UIPopover - because the error sometimes cause it to be | ||
| // unmounted. This is an unfortunate hack. | ||
| setPopoverKey(prev => prev + 1) |
There was a problem hiding this comment.
Hmm, I'm not 100% confident that this is handling the situation correctly. Have you been able to trigger the SecurityError somehow? The previous implementation used an error boundary, which also prevented the error from bubbling up to the window. Error boundaries are not supported by functional components. This will just react to an error in the window, but this error might cause other issues in the application itself.
frontend/lib/src/components/shared/BaseColorPicker/BaseColorPicker.tsx
Outdated
Show resolved
Hide resolved
frontend/lib/src/components/shared/BaseColorPicker/BaseColorPicker.tsx
Outdated
Show resolved
Hide resolved
...laywright/__snapshots__/linux/st_sidebar_test/st_sidebar-date_popover[dark_theme-webkit].png
Show resolved
Hide resolved
|
@lukasmasuch I think this is ready for another review. I fixed a few additional things along the way (especially with color picker styling), so would be good if you could look through everything again. I'll talk with Jessi about making the dropdowns look more distinguishable in dark mode but I might do that in a follow-up PR. |
There was a problem hiding this comment.
I assume this is expected that time_input now shows more options?
There was a problem hiding this comment.
Yup, I increased the max height there to be the same as all other dropdowns.
There was a problem hiding this comment.
And the scroll to the selected item on open still works in this case?
There was a problem hiding this comment.
Just verified, yup this still works!
…` to functional component (streamlit#10083) ## Describe your changes Many of our widgets have dropdowns but they look slightly different. This PR makes them look more consistent. While I was at it, I also converted `BaseColorPicker` to a functional component because I couldn't figure out how to fix some issues with theming in the class-based component. The only things that changed in the color picker are the injection of custom styles and the placement of the popover, so I think doing the conversion at the same time shouldn't be too confusing. But I can separate both changes if it's easier to review. | Before | After | |----------|----------| |  |  | |  |  | |  |  | |  |  | |  |  | Note that the date input has a red border and is in the foreground when focused (just like the other components), my screenshot tool somehow removes that, though. ## GitHub Issue Link (if applicable) Closes streamlit#8705 ## Testing Plan - Updated snapshot tests for date input. - I added a snapshot test for the dropdown of color picker since that has a few customizations now. - I realized we don't really have snapshot tests for the dropdown elements of the other widgets. One problem is though that the snapshots test only capture the content of the dropdown, not the shadow and positioning around it. *Ideally*, we'd have some way to take a snapshot with some additional padding around it but I'm not sure if it's worth introducing that. - Don't think it makes sense to have unit tests for such small styling things. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license. --------- Co-authored-by: Johannes Rieke <[email protected]>
Describe your changes
Many of our widgets have dropdowns but they look slightly different. This PR makes them look more consistent.
While I was at it, I also converted
BaseColorPickerto a functional component because I couldn't figure out how to fix some issues with theming in the class-based component. The only things that changed in the color picker are the injection of custom styles and the placement of the popover, so I think doing the conversion at the same time shouldn't be too confusing. But I can separate both changes if it's easier to review.Note that the date input has a red border and is in the foreground when focused (just like the other components), my screenshot tool somehow removes that, though.
GitHub Issue Link (if applicable)
Closes #8705
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.