Fix plotly height attributes and useContainerWidth #5779
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…h=False for both themes
willhuang1997
commented
Nov 29, 2022
willhuang1997
commented
Nov 29, 2022
willhuang1997
commented
Nov 29, 2022
willhuang1997
commented
Nov 29, 2022
willhuang1997
commented
Nov 29, 2022
frontend/src/components/elements/PlotlyChart/PlotlyChart.test.tsx
Outdated
Show resolved
Hide resolved
AnOctopus
reviewed
Nov 29, 2022
AnOctopus
reviewed
Nov 29, 2022
AnOctopus
reviewed
Nov 29, 2022
AnOctopus
reviewed
Nov 29, 2022
AnOctopus
reviewed
Nov 29, 2022
AnOctopus
approved these changes
Nov 29, 2022
|
|
||
| // Update config and spec references iff the theme or props change | ||
| useEffect(() => { | ||
| useLayoutEffect(() => { |
Contributor
There was a problem hiding this comment.
since useLayoutEffect is unusual, consider adding a comment about why we use it instead of useEffect, or include a link to an explanation
tconkling
added a commit
to tconkling/streamlit
that referenced
this pull request
Nov 30, 2022
* feature/ReleaseCacheV2: Rename: cache_data, cache_resource (streamlit#5785) Pin pyflakes in autoflake hook (streamlit#5790) Fix plotly height attributes and useContainerWidth (streamlit#5779)
kmcgrady
pushed a commit
that referenced
this pull request
Nov 30, 2022
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📚 Context
When I was fixing the fullscreen bug where the PlotlyChart component was not properly keeping state of width and height, I defaulted the height and width to incorrect heights and widths. Because of this, I broke the useContainerWidth parameter and when setting height to the chart. In addition, in order to truly solve the full screen issue, we need to use
useLayoutEffectinstead ofuseEffect.Explanation about both can be found here: https://kentcdodds.com/blog/useeffect-vs-uselayouteffect
Testing matrix and methodology:
Goal: making sure the coverage on testing is truly correct for height / width changes.
In 2 of the e2e tests, we have the height and width manually changed with each theme.
Testing for fullscreen which is done manually and through some unit tests.
Testing for useContainerWidth for false, true are also in unit tests.
Lmk if I am missing any other tests.
Please describe the project or issue background here
What kind of change does this PR introduce?
🧠 Description of Changes
Add bullet points summarizing your changes here
Revised:
Insert screenshot of your updated UI/code here
Current:
Insert screenshot of existing UI/code here
🧪 Testing Done
🌐 References
Does this depend on other work, documents, or tickets?
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.