Conversation
- use_container_width. - small refactor. - bugfix: DeckGLChart where not re-rendering if the props changes. - adding deprecation warning message
tconkling
left a comment
tconkling
left a comment
There was a problem hiding this comment.
Looks good. I left a few nitpicky comments. Also, there are no tests; I can't remember if we have a convention around testing everything that can be marshalled from Python into Protobufs, but it might be worth adding some simple assertions to graphviz_test and deck_gl_test, to make sure that the flag is passed through as expected.
| if (height) { | ||
| //fullscreen | ||
| json.initialViewState.height = height | ||
| json.initialViewState.width = width | ||
| } else { | ||
| if (!json.initialViewState.height) { | ||
| json.initialViewState.height = DEFAULT_DECK_GL_HEIGHT | ||
| } | ||
|
|
||
| if (useContainerWidth) { | ||
| json.initialViewState.width = width | ||
| } | ||
| } |
There was a problem hiding this comment.
If height == null && !useContainerWidth, then initialViewState.width won't be set. Is that intentional? Can you add a comment just explaining why initialViewState.height/width are not always set? (Are they already set in the JSON and this is just overriding them?)
|
|
||
| json.initialViewState.height = height | ||
| json.initialViewState.width = width | ||
| if (height) { |
There was a problem hiding this comment.
Nit: I think this if (height) would be better expressed either as a getter called this.isFullscreen(), or maybe just a local variable called isFullscreen that does the same, with documentation. It's very non-obvious (and easy to miss) why height being defined means that the chart is fullscreen.
| pitch: number | ||
| bearing: number | ||
| zoom: number | ||
| state = { |
There was a problem hiding this comment.
I'm not sure if there's a Streamlit convention around this or not, but I think since the rest of this function uses public/private access modifiers, state and componentDidMount() should be explicitly marked as private.
There was a problem hiding this comment.
I'm getting a type error if I put those in private
TS2415: Class 'DeckGlJsonChart' incorrectly extends base class 'PureComponent<PropsWithHeight, State, any>'. Property 'componentDidUpdate' is private in type 'DeckGlJsonChart' but not in type 'PureComponent<PropsWithHeight, State, any>'.
TS2415: Class 'DeckGlJsonChart' incorrectly extends base class 'PureComponent<PropsWithHeight, State, any>'. Property 'state' is private in type 'DeckGlJsonChart' but not in type 'PureComponent<PropsWithHeight, State, any>'.
There was a problem hiding this comment.
My mistake; I didn't check the subclass visibility. My point was that we should use visibility modifiers consistently, so if these need to be public, then they should be explicitly marked as such.
1159/use_container_width (streamlit#1174)
* develop: Unpin python-dateutil package version (streamlit#1153) Pypi nightly builds (streamlit#1171) Adding custom filterOptions function to SelectBox (streamlit#1182) Expire media files when ReportSession expires (streamlit#1128) 1159/use_container_width (streamlit#1174)
2 participants
Issue: Fixes #1159 #1045
Contribution License Agreement
By submiting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.