-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add internal skeleton element #8074
Add internal skeleton element #8074
Conversation
… metrics. Fix js tests by creating AppSkeleton with a SkeletonProto rather than a raw height parameter.
I think I would like to make this switch between the "Skeleton" and "AppSkeleton" react components depending on the height specified, but I am not sure of the best place to put that conditional code -- it would have to be in ElementNodeRenderer but that seems to break the single-responsiblity principle. Would it be better to implement a "SkeletonElement" component which selectively renders either an "AppSkeleton" or a "Skeleton" component based on height? |
Thanks for the contribution @Asaurus1! I should have the time to give this a proper review sometime in the next few days. I'm in favor of accepting this change as currently implemented in the PR (I'm less certain about the fancier callback behavior described in #8032, but I have no reservations around the behavior that's essentially just |
@vdonato You're welcome! I certainly don't think the callback stuff is necessary, especially with the new st.partial stuff that is in the works. I would like to do some experiments with small and large heights for the skeleton, as well as make sure it works graphically inside things like expanders. And if the UI folks have any input on looks that's welcome as well. EDIT: I got a chance to perform these tests and everything looks clean as far as I can see. Made a cleanup to the docstring for the python function and I think it's good to go pending any additional feedback. |
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.
This more or less looks good to me on the eng side (we'll still need approval from the product team whenever they're able to get around to it before this can be submitted, though). Just a few things to bring up:
- As mentioned in some other comments, the seemingly arbitrary default skeleton height value of 107 feels a bit wonky. I'm not entirely sure what to do about this, though 😕. Can surface this to the rest of the team to see if anyone that's better at styling things than me (so.. everyone) has any ideas
- It'd be great to have some e2e playwright tests for this feature. I think some reasonably comprehensive ones shouldn't be too difficult to implement as playwright should provide the primitives to verify that a skeleton appears and is eventually replaced by some content (and the script itself could just pause for a few seconds before
st.write
-ing a message)
Maybe it'd be best to hold off on addressing these for a bit until product responds just to avoid any churn if they're more hesitant to accept this feature
@sfc-gh-dmatthews or @sfc-gh-jcarroll, I noticed that the Skeleton boxes and the text elements have different radii of rounded corners. I know the overall streamlit theming changed in a recent release (like 1.26 or something), I'm wondering if this was overlooked or intentional. Would be good to have one of your UI folks take a look, maybe. |
…prate proto field to allow it to remain unchanged.
33c43a4
to
c718533
Compare
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.
This already looks pretty good. But we still need some feedback from our product team (e.g. @jrieke). And I'm pretty certain that the width
parameter will not be desired for the first release of this command. Especially since the width
here works differently (relative width) to all the other width
parameters in Streamlits API. That would take some extra discussions.
@snehankekre all great points! I'm happy to rebase this to just the JS side of things and keep the python stuff around in a separate branch to be sprinkled in again later if we desire. Regarding the first "pop-in" concern, I think that can be fairly easily mitigated with a DelayedSkeleton component, which simply draws itself as an empty in the application for the first 0.5 seconds (similar to how the app skeleton works now) and only displays if that time has elapsed and it hasn't already been replaced. The second concern about height and when this should be available is definitely more difficult to solve and deserves additional consideration. I can imagine some kind of fancy front end magic that detects when a skeleton is getting replaced and, computes the height of the new content, and then does some kind of CSS transition of the skeleton to the new content height, but that is definitely beyond the scope of this pull request, if it's even technically possible. |
Sounds like a great solution to put in the JS hooks and expose the API in streamlit-extras for now. Thanks @Asaurus1 for building this and working with us to drive it through! |
Sounds good 👍 So we need the JS stuff + the |
I think this would be a good idea. But I think it doesn't resolve one important problem we have with our placeholder e.g. import streamlit as st
import time
placeholder = st.empty()
time.sleep(5)
placeholder.dataframe(["A", "B", "C"]) The dataframe in this example will disappear on every rerun for 5 seconds. While in this example it will just go stale (grey out): import streamlit as st
import time
time.sleep(5)
st.dataframe(["A", "B", "C"]) This is even more problematic if you set the sleep to 1 second in both examples. The one without a placeholder will not show any visible change happening to the user while the placeholder usage will have it disappear. There are also other issues connected to this, e.g. the component might lose state. The reason why this is happening is the way of how we process the delta tree in the frontend and how we invalidate existing frontend components. We probably should start with finding a good solution for |
3ec1b09
to
7a453d7
Compare
Ok I think everything is passing on Asaurus1#3 now so it should pass here. |
@snehankekre any blockers to merging this? If we're still waiting on product team for the js-only changes that's alright. |
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.
LGTM 👍 But I will double-check if we want to keep the width attribute in as well. We might potentially remove this before the release.
@@ -336,11 +338,12 @@ function ComponentInstance(props: Props): ReactElement { | |||
// but while we also have not waited until the ready timeout | |||
const loadingSkeleton = !isReadyRef.current && | |||
!isReadyTimeout && | |||
// if height is explicitly set to 0, we don’t want to show the skeleton at all |
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.
nit: the comment is still true, or? I think it makes sense to keep this comment.
frontend/lib/src/components/widgets/CustomComponent/ComponentInstance.tsx
Show resolved
Hide resolved
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
…reamlit-skeletons
// Skeleton visual style | ||
SkeletonStyle style = 1; | ||
|
||
// Height in CSS points |
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.
Nit: this is pixels, not points.
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.
oh, added a fix to this PR: #8508
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.
To be fair, I knew this was wrong when I wrote it but I went for consistency because I looked at other code in the repo (other protos) and it all says "css points" for things that appear to be measured in pixels. It's totally possible i misread the other stuff tho.
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.
Hah, then we should fix those too at some point 😆
Either way, thanks for the contribution @Asaurus1 !
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.
#8508 fixes those wrong "css points" comments as well 👍
Describe your changes
Implements user-callable "skeleton" blocks (behave as empties)
GitHub Issue Link (if applicable)
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.