Skip to content
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

Fix flickering effect when changing tabs #7904

Merged
merged 2 commits into from Jan 4, 2024

Conversation

LukasMasuch
Copy link
Collaborator

@LukasMasuch LukasMasuch commented Jan 4, 2024

Describe your changes

I don't know when and why the width == 0 flicker problem returned (could be related to this or this change or some browser changes). It seems to happen a bit more randomly:

Screen.Recording.2024-01-04.at.21.09.46.mov

The fixes in this PR seem to prevent this flickering fully.

Testing Plan

  • Not really easy to test since it is just some flickering.

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-lmasuch sfc-gh-lmasuch changed the title Fix flickering effects with tabs Fix flickering effects related to observer width==0 Jan 4, 2024
@sfc-gh-lmasuch sfc-gh-lmasuch changed the title Fix flickering effects related to observer width==0 Fix flickering effect when changing tabs Jan 4, 2024
@@ -136,8 +136,7 @@ export const StyledVerticalBlock = styled.div<StyledVerticalBlockProps>(
({ width, theme }) => ({
width,
position: "relative", // Required for the automatic width computation.

display: "flex",
display: width === 0 ? "none" : "flex",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might not even need it. But I think in any case, if the width of a vertical container is 0, we should better not display the container. So, it might be a good precaution for undetected issues.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm lukewarm to this, I guess the issues that could pop up is "my content is not displayed" vs "my content flickers or is mislayout". I think it's probaly good enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Yeah, I think it could make some of the very edge cases harder to detect. I have removed this again. If we see some related flickering in the future, we could decide to add this again.

@LukasMasuch LukasMasuch marked this pull request as ready for review January 4, 2024 21:45
@@ -136,8 +136,7 @@ export const StyledVerticalBlock = styled.div<StyledVerticalBlockProps>(
({ width, theme }) => ({
width,
position: "relative", // Required for the automatic width computation.

display: "flex",
display: width === 0 ? "none" : "flex",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm lukewarm to this, I guess the issues that could pop up is "my content is not displayed" vs "my content flickers or is mislayout". I think it's probaly good enough.

@LukasMasuch LukasMasuch merged commit 51ca2e9 into develop Jan 4, 2024
44 checks passed
zyxue pushed a commit to zyxue/streamlit that referenced this pull request Apr 16, 2024
* Fix flickering effects

* Remove display none
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants