Fullscreen button to expand tables and charts by dcaminos · Pull Request #137 · streamlit/streamlit

dcaminos · 2019-09-18T19:21:27Z

*Added new component FullScreenWrapper

jrhone · 2019-09-19T18:18:33Z

I'm seeing a slight flicker of the button after expanding and resetting.

To reproduce try: expand dataframe -> move mouse below chart -> press escape

frontend/src/components/elements/Chart/Chart.jsx

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.tsx

jrhone · 2019-09-19T18:35:10Z

frontend/src/components/core/Block/Block.tsx

+      table: (el: SimpleElement) => (
+        <FullScreenWrapper width={width}>
+          {({ width, height }) => (
+            <Table element={el} width={width} height={height} />


I don't think table is responding to the height property.

No, and width either, and I don't fixed it because I finded this comment on the code:

streamlit/frontend/src/components/elements/Table/Table.jsx

Line 40 in 9d049f6

// TODO(tvst): Make tables have a max width with overflow: scroll (when

@tvst can you clarify this please?

@tvst can you comment on why table is not handling width or height?

Tables don't have a height prop. So you should just remove it here.

As for width, I don't really recall what's up with that comment I wrote. The comment is now almost 1 year old, though, so you can't blame me! 😎

tvst

Just tried this out on my machine. It's so awesome 👏

Some comments about the UX:

Can you add the "expand" button to all types of plots? This means VegaLiteChart.tsx, DeckGlChart.tsx, GraphVizChart.tsx, PlotlyChart.tsx, and BokehChart.tsx
Can you add the "expand' button to images ~~and videos~~ too? ImageList.tsx ~~and Video.tsx~~. Edit: actually videos already have a fullscreen button. No need to do anything there.
Add a tooltip on the button, that says "View fullscreen" and "Exit fullscreen"
When I go fullscreen and then rerun the app (by pressing r), the UI looks weird. I think it's because the fullscreen element is fading away. As a quick fix, can you make elements not fade when fullscreen?

Also, please run make headers to add the Apache header to all new files.

tvst · 2019-09-19T19:05:58Z

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.scss

@@ -0,0 +1,32 @@
+.streamlit-container .stFullScreenFrame--expanded {
+  position: fixed;
+  top: 0px;


Use 0 instead of 0px. Same below.

tvst · 2019-09-19T19:50:40Z

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.scss

+  overflow: auto;
+  background: white;
+  z-index: 1000;
+  padding: 30px;


Can we make this smaller? Like 0.5rem?
(When I do this I still get a huge margin on the right and bottom sides. No idea why.)

tvst · 2019-09-19T19:50:43Z

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.scss

+  background: white;
+  z-index: 1000;
+  padding: 30px;
+  padding-top: 80px;


Use the same value as the header height here. I believe that's 3.5rem (which includes some margin).

Also, please put 3.5rem as a variable in variables.scss and use it in header.scss and here.

tvst · 2019-09-19T19:52:14Z

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.scss

+.streamlit-container .stFullScreenFrame .stButton-enter {
+  position: absolute;
+  right: 10px;
+  top: 10px;


Look at header.scss and see what we use for the hamburger menu, make it a variable and use it here.

(Also, I think we don't use px as a unit for this things, but I may be mistaken...)

dcaminos · 2019-09-20T21:30:13Z

@tvst should be better don't re run when you are in fullscreen?

"When I go fullscreen and then rerun the app (by pressing r), the UI looks weird. I think it's because the fullscreen element is fading away. As a quick fix, can you make elements not fade when fullscreen?"

tvst · 2019-09-20T21:46:32Z

When I go fullscreen and then rerun the app (by pressing r), the UI looks weird. I think it's because the fullscreen element is fading away. As a quick fix, can you make elements not fade when fullscreen?

@tvst should be better don't re run when you are in fullscreen?

In terms of user experience, I think it's best to allow rerunning when fullscreen. This way you if you have a time-varying script, you can zoom into a chart and keep rerunning the app to focus on how the reruns impact the chart.

But if that is too much work to get right (more than an afternoon), then it's OK to disallow it for now.

… PlotlyChart, BokehChart (now responsive) and ImageList

frontend/src/components/elements/ImageList/ImageList.tsx

frontend/src/components/core/Block/Block.tsx

frontend/src/components/elements/BokehChart/BokehChart.tsx

frontend/src/components/elements/GraphVizChart/GraphVizChart.tsx

jrhone · 2019-09-24T15:35:49Z

frontend/src/components/elements/GraphVizChart/GraphVizChart.tsx

  public render = (): React.ReactNode => {
    const width: number = this.props.element.get("width") || this.props.width
-
+    const height: number =


Shouldn't this use the same logic as the code block above for // Override or reset the graph height?

Is the same logic, this.props.element.get("height") have preference

Above also uses this.originalHeight .. should it be a possible value for height here as well?

frontend/src/components/elements/PlotlyChart/PlotlyChart.tsx

tvst · 2019-09-24T16:30:23Z

I just created a branch tweaking the styles for these new buttons to match the "copy" button that shows up when you hover over codeblocks. I also moved the buttons to the right of the elements rather than on top.

Please take a look and merge into your branch:
https://github.com/dcaminos/streamlit/compare/issue395/expand_to_fullscreen...tvst:issue395/tvst-styles?expand=1

frontend/src/components/elements/DataFrame/DataFrame.tsx

jrhone · 2019-09-24T17:06:01Z

frontend/src/components/elements/GraphVizChart/GraphVizChart.tsx


 interface Props {
  width: number
+  height: number | undefined


Unrelated to this line but the UI is a bit jumpy when expanding and resetting the graphviz charts (and probably other charts) but it's easy to see with the second chart in e2e/scripts/graphviz_chart.py

Yes, I agree about graphviz (no others charts). I guess is because D3 update the chart after component did update.

jrhone · 2019-09-24T17:31:41Z

frontend/src/components/elements/VegaLiteChart/VegaLiteChart.tsx


 interface Props {
  width: number
+  height: number | undefined


I'm not seeing the expand button on the first 4 vega lite charts in e2e/scripts/vega_lite_chart.py, only the last 4, is this expected?

The charts expand to fullscreen height but not width, is this expected?

If you expand a chart, then set the width to 500, then re-run report, the chart will expand to the width but revert it's height. If you set the width before expanding the chart, it expands correctly. Very minor case.

I'm not seeing the expand button on the first 4 vega lite charts

You are right, there are a bug with the opacity transition, I will try to solve it.

The charts expand to fullscreen height but not width, is this expected?

That is right, because the user are specifying a width into the script

If you expand a chart, then set the width to 500, then re-run report, the chart will expand to the width but revert it's height. If you set the width before expanding the chart, it expands correctly. Very minor case.

I will try to solve it

tvst

Looks good to me, after comments are addressed.

But since Jonathan left comments too, I'll let him be the one to click "approve"

frontend/src/components/elements/BokehChart/BokehChart.tsx

tvst · 2019-09-26T03:02:13Z

frontend/src/components/elements/BokehChart/BokehChart.tsx

  index: number
 }

+const DEFAULT_HEIGHT = 400


Does this mean that when in normal non-fullscreen mode the default height for a Bokeh chart will be 400px? If so, I think we shouldn't do that. Instead, just let Bokeh choose the height of the chart. This way Bokeh charts in Streamlit will work just like Bokeh charts anywhere else.

Same with the other charts below.

Before this PR, all Bokeh charts was 600px x 600px. Users must write the chart width and height on python code to set that values.

If we dont set a default, the height will be 600px. We can set the default on 600 or just remove the default value.

Ah I see. Yeah, I think removing the default makes sense. This way Bokeh decides what to use as default.

Same for other libraries in this PR.

frontend/src/components/elements/Chart/Chart.jsx

tvst · 2019-09-26T03:19:28Z

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.tsx

+
+  componentDidMount() {
+    this.updateWindowDimensions()
+    window.addEventListener("resize", this.updateWindowDimensions)


Nice! Good thinking

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.tsx

tvst · 2019-09-26T03:24:19Z

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.tsx

+
+  updateWindowDimensions = () => {
+    this.setState({
+      fullwidth: window.innerWidth - this.convertRemToPixels(1.5), //0.75rem + 0.75rem


Where is the 1.5 coming from? If we have it variables.scss then you can get it here with import { SCSS_VARS } from "autogen/scssVariables".

Otherwise, at least add a comment explaining where it comes from. And if there is some CSS file that this needs to be in sync with, add a comment in that file pointing to this.

Same below.

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.tsx

jrhone · 2019-09-27T16:05:26Z

frontend/mochawesome.json

@@ -0,0 +1,283 @@
+{


Remove file from PR and add to .gitignore

jrhone · 2019-09-27T16:21:19Z

frontend/src/components/core/Block/Block.tsx

+      table: (el: SimpleElement) => (
+        <FullScreenWrapper width={width}>
+          {({ width, height }) => (
+            <Table element={el} width={width} height={height} />


@tvst can you comment on why table is not handling width or height?

jrhone · 2019-09-27T18:24:15Z

frontend/src/components/elements/GraphVizChart/GraphVizChart.tsx

  public render = (): React.ReactNode => {
    const width: number = this.props.element.get("width") || this.props.width
-
+    const height: number =


Above also uses this.originalHeight .. should it be a possible value for height here as well?

jrhone · 2019-09-27T18:37:13Z

The expand icon is staying visible on a Vega lite chart after minimizing. It doesn't seem to disappear until you click somewhere off the chart. Is it expected?

dcaminos · 2019-09-30T21:02:58Z

The expand icon is staying visible on a Vega lite chart after minimizing. It doesn't seem to disappear until you click somewhere off the chart. Is it expected?

@jrhone I saw it. I'm not sure if this is wrong. Is like a make, like "you was here"

But we can talk about this if you want.

dcaminos · 2019-09-30T22:09:34Z

@tvst this is the bug I fixed with vega lite charts:

tvst · 2019-09-30T23:07:54Z

frontend/src/components/elements/VegaLiteChart/VegaLiteChart.tsx

+    spec.width = width
+    spec.height = height
+    spec.padding = {
+      bottom: EMBED_PADDING,


Wait, this will override any padding the user adds, right?

Yes. Can they set a padding when write the chart? I that case, I can use that padding and this should be the default one.

dcaminos · 2019-10-01T19:20:16Z

@tvst I improved the vega-lite solution to do not override current padding

…ING_BOTTOM)

tvst

Leaving another partial review, as I haven't looked at everything yet. Will look through it all by tomorrow at noon. Apologies for the delay!

tvst · 2019-10-03T06:16:18Z

frontend/src/components/core/Block/Block.tsx

+              height={height}
+            />
+          )}
+        </FullScreenWrapper>


Can you move these FullScreenWrappers into each element's component? It would be nice to keep this big switch statement lean.

tvst · 2019-10-03T06:40:00Z

frontend/src/components/core/Block/Block.tsx

+      table: (el: SimpleElement) => (
+        <FullScreenWrapper width={width}>
+          {({ width, height }) => (
+            <Table element={el} width={width} height={height} />


Tables don't have a height prop. So you should just remove it here.

As for width, I don't really recall what's up with that comment I wrote. The comment is now almost 1 year old, though, so you can't blame me! 😎

tvst · 2019-10-03T06:42:57Z

frontend/src/components/shared/FullScreenWrapper/FullScreenWrapper.tsx

+
+    return (
+      <div className={`stFullScreenFrame${expanded ? "--expanded" : ""}`}>
+        <button


Is is possible to use a Reactstrap <Button> component? See the main menu and toolbar button for examples.

Yes I can, but I based on this button: https://github.com/dcaminos/streamlit/blob/58bd06cee4de0b403dbfff55de8a07aea79dbf4a/frontend/src/components/elements/CodeBlock/CopyButton.tsx#L46

We should create a "OverlayButton" component for both cases (and use Reactstrap), but I don't wanna do it on this PR (it's too big right now)

Sounds good. Can you create an issue for this?

tvst · 2019-10-08T19:22:40Z

There are some JS warnings, by the way:

./src/components/elements/CodeBlock/CopyButton.tsx
  Line 20:  'Button' is defined but never used  @typescript-eslint/no-unused-vars

./src/components/elements/VegaLiteChart/VegaLiteChart.tsx
  Line 29:  'max' is defined but never used  @typescript-eslint/no-unused-vars

* develop: Fullscreen button to expand tables and charts (streamlit#137) Blacklist for hashing (streamlit#254) Adding utils and tests for hello.py (streamlit#261) Fix port-finding bugs (streamlit#280) Fix hello.py usage of os.path.join on a URL (streamlit#285)

add new component FullScreenWrapper

cac0b69

dcaminos requested review from arraydude, kantuni and tvst September 18, 2019 19:21

Daniel Caminos added 3 commits September 18, 2019 16:28

fix DataFrame height

13ebeec

block scroll on modal

7cb6d06

add escape to exit fullscreen

6f3cb36

jrhone reviewed Sep 19, 2019

View reviewed changes

tvst reviewed Sep 19, 2019

View reviewed changes

add fullscreen feature for VegaLiteChart, DeckGlChart, GraphVizChart,…

b44c534

… PlotlyChart, BokehChart (now responsive) and ImageList

dcaminos requested review from jrhone and tvst September 23, 2019 21:13

Daniel Caminos and others added 4 commits September 23, 2019 18:23

bokeh width and height are optional

fcbc618

Merge branch 'develop' into issue395/expand_to_fullscreen

7827414

fix test

1d5d824

add validation to BokehChart

4234bad

tvst reviewed Sep 24, 2019

View reviewed changes

frontend/src/components/elements/ImageList/ImageList.tsx Outdated Show resolved Hide resolved

jrhone reviewed Sep 24, 2019

View reviewed changes

Update overlay button styles.

054ea67

jrhone reviewed Sep 24, 2019

View reviewed changes

frontend/src/components/elements/DataFrame/DataFrame.tsx Outdated Show resolved Hide resolved

jrhone reviewed Sep 24, 2019

View reviewed changes

improve code

51fe3de

tvst reviewed Sep 26, 2019

View reviewed changes

add open-iconic font and run make headers

e1b041b

implemment SCSS_VARS values to calculate fullscreen dimension

cebf784

dcaminos requested review from jrhone and tvst September 26, 2019 19:03

jrhone approved these changes Sep 27, 2019

View reviewed changes

Daniel Caminos added 3 commits September 30, 2019 17:01

merge develop into my branch

5356a83

improve vega lite view

9e43c55

remove comment

08b145e

Daniel Caminos added 2 commits September 30, 2019 19:04

remove default height

0eb6828

update ignore_cache with ignore_hash on lib/streamlit/hashing.py

36d700a

tvst reviewed Sep 30, 2019

View reviewed changes

improve vega-lite padding solution

e278c9d

Daniel Caminos added 3 commits October 1, 2019 16:22

fix wrong symbol

a199953

improve logic with padding.bottom = Math.max(padding.bottom, FIX_PADD…

37b4a9c

…ING_BOTTOM)

delete magic.py from examples

7066550

tvst reviewed Oct 3, 2019

View reviewed changes

Daniel Caminos added 3 commits October 7, 2019 16:16

remove FullScreenWrapper from Block switch

b761ca6

remove height prop from Table

c39958b

merge develop

58bd06c

dcaminos requested a review from tvst October 7, 2019 19:51

tvst approved these changes Oct 8, 2019

View reviewed changes

Daniel Caminos added 2 commits October 8, 2019 16:55

remove warnings

ccc8841

delete unused files

f9ffcd2

dcaminos merged commit ed9dd36 into streamlit:develop Oct 8, 2019

dcaminos deleted the issue395/expand_to_fullscreen branch October 8, 2019 20:50

Conversation

dcaminos commented Sep 18, 2019

Uh oh!

jrhone commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcaminos commented Sep 20, 2019

Uh oh!

tvst commented Sep 20, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tvst commented Sep 24, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrhone Sep 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcaminos Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jrhone commented Sep 19, 2019 •

edited

Loading

tvst left a comment •

edited

Loading

jrhone Sep 24, 2019 •

edited

Loading

dcaminos Sep 25, 2019 •

edited

Loading

jrhone commented Sep 27, 2019 •

edited

Loading