Collapse button for floating viz controls #535
Conversation
There was a problem hiding this comment.
Looks great. A few small comments in the code.
If it's trivial, without massively reengineering coreui, would you be able to make the new button a bit smaller? Ideally not larger than the Add filters button in the header, and maybe not all caps also. (this is the only firm "requested change" - others are for discussion)
There is also a more general problem of our plots (EDA and SAM) all wasting space at the top where a header can go (IIRC, we add a header when faceting in regular SAM).
We should talk about a follow-on ticket that considers the window sizing.
I've added a Map UX topic (item 3 here) to decide what's best for state management. Window size and location could perhaps be part of that discussion.
| @@ -66,6 +67,8 @@ interface Props { | |||
| createComputeJob?: () => void; | |||
| /** optional dynamic plugins */ | |||
| plugins?: Partial<Record<string, ComputationPlugin>>; | |||
| hideInputsAndControls?: boolean; | |||
| plotContainerStylesOverrides?: PlotContainerStylesOverrides; | |||
There was a problem hiding this comment.
This is pedantic, sorry, but we've called these types of prop XyzStyleOverrides, not XyzStylesOverrides elsewhere...
| interface Props { | ||
| selectedOverlayConfig?: OverlayConfig | BubbleOverlayConfig; | ||
| overlayHelp?: ReactNode; | ||
| vizOptions?: StandaloneVizOptions; |
There was a problem hiding this comment.
Was this prop planned and never used?
| styleOverrides={{ | ||
| zIndex: zIndexForStackingContext, | ||
| resize: 'both', | ||
| overflow: 'hidden', |
There was a problem hiding this comment.
tbh I don't fully understand the overflow property usage here.
It may be a browser difference (though I've tried Chrome and Firefox).
I am seeing scroll bars appear on mouse-over. Perhaps you could explain to me later? Sorry!
There was a problem hiding this comment.
We want it to scroll, but we don't want DraggablePanel to handle the scrolling (which it does by default); we want to handle it ourselves in the divs we pass as children of DraggablePanel. Thus you'll see overflow: 'auto' on one of the child divs below.
| styleOverrides={{ | ||
| zIndex: zIndexForStackingContext, | ||
| resize: 'both', | ||
| overflow: 'hidden', |
| }} | ||
| > | ||
| <FilledButton | ||
| text={hideInputsAndControls ? 'Show Controls' : 'Hide Controls'} |
There was a problem hiding this comment.
| text={hideInputsAndControls ? 'Show Controls' : 'Hide Controls'} | |
| text={hideInputsAndControls ? 'Show controls' : 'Hide controls'} |
Call me a pedant, but this capitalisation would match the other text on screen - "Box plot" and "Add filters" (but not "Unnamed Analysis"...). Not a hill I'm willing to die on though.
Closes #99