Use toolbar for fullscreen button #9721
Conversation
sfc-gh-nbellante
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
frontend/lib/src/components/elements/ArrowVegaLiteChart/ArrowVegaLiteChart.tsx
Fixed
Show fixed
Hide fixed
sfc-gh-nbellante
added
security-assessment-completed
sfc-gh-nbellante
force-pushed
the
nico/vega-fullscreen-button-to-toolbar
branch
from
13bb7d3 to
a9a03d0
Compare
sfc-gh-nbellante
changed the title
sfc-gh-nbellante
changed the title
sfc-gh-bnisco
left a comment
sfc-gh-bnisco
left a comment
There was a problem hiding this comment.
Looking really good! Nice job simplifying this code even further! I have a few small suggestions inline.
frontend/lib/src/components/elements/ArrowVegaLiteChart/styled-components.ts
Outdated
Show resolved
Hide resolved
sfc-gh-bnisco
left a comment
sfc-gh-bnisco
left a comment
There was a problem hiding this comment.
Code LGTM, thanks for the iterations!
There was a problem hiding this comment.
I think you can remove all the traces of StyledFullScreenButton within this file since it is not anymore used.
There was a problem hiding this comment.
good callout -- i wonder i there's a way to get eslint to complain about that 🤔
There was a problem hiding this comment.
i could not find a good way. i found a project that uses typescripts ast (ts-prune) but it was so slow i couldn't even get it to finish on a subset of our repo :(
lukasmasuch
left a comment
lukasmasuch
left a comment
There was a problem hiding this comment.
LGTM 👍 One potential small simplification: the toolbar component itself could also use context to correctly set the onExpand, onCollapse and isFullscreen for the fullscreen toolbar action button (if disableFullscreenMode == False). This would allow us to remove these props from Toolbar itself.
…hat was breaking tests
sfc-gh-nbellante
force-pushed
the
nico/vega-fullscreen-button-to-toolbar
branch
from
45decc5 to
d2cb807
Compare
My concern with doing that is that the toolbar then could only be used when it's a descendent of that context provider. Which, right now the only component that would break is the AudioInput, but it felt like a confusing coupling to make. Wdyt? Edit: I guess we could modify things such that we allow the context not being there when the disableFullscreenMode is true? |
Yep, I think we could only use the context within toolbar if the fullscreen mode is actually active. The other alternative might be to have a base Toolbar Component that doesn't include any fullscreen aspects and a FullscreenToolbar that uses Toolbar and adds the toolbar action + uses the context. |
## Describe your changes This PR: - removes the old `FullScreenWrapper` - changes the `withFullScreenWrapper` to instead use the newly created `ElementFullscreenWrapper` - brings fullscreen toolbar functionality and standardizes pattern across `DeckGIJsonChart`, `ArrowVegaLiteChart`, `ImageList`, `PlotlyChart`, `DataFrame`  ## Testing Plan automated tests updated --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
Describe your changes
This PR:
FullScreenWrapperwithFullScreenWrapperto instead use the newly createdElementFullscreenWrapperDeckGIJsonChart,ArrowVegaLiteChart,ImageList,PlotlyChart,DataFrameTesting Plan
automated tests updated
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.