feat: Adjustable grid density#2151
Conversation
|
What about "Default table density"? Implying ui.table() density overrides default density, similar to "Default formatting" |
| }; | ||
| webgl: boolean; | ||
| webglEditable: boolean; | ||
| gridDensity?: 'compact' | 'normal' | 'spacious'; |
There was a problem hiding this comment.
This should always be set. Also we may as well match the Spectrum density props, which use regular instead of normal (though not every component has a spacious option).
| gridDensity?: 'compact' | 'normal' | 'spacious'; | |
| gridDensity: 'compact' | 'regular' | 'spacious'; |
Then define what the default value is in LocalWorkspaceStorage.
In the updateWorkspaceData action, we map the users CustomizableWorkspaceSettings on top of the default settings. That way the default values are always set in the WorkspaceSettings we get from redux, and there's no ambiguity on what the "default" value is.
Side note, we should probably allow this to be set via a prop as well (for a default value). Can be a separate ticket.
There was a problem hiding this comment.
Ya I initially made it optional since it wouldn't be set in enterprise right away, but shouldn't matter until we add the settings menu in enterprise
Should the server prop be DHC or just DHE?
There was a problem hiding this comment.
Would be nice to add to both but no urgency for DHC
| const settings = useSelector<RootState, ReturnType<typeof getSettings>>( | ||
| getSettings | ||
| ); | ||
| const dispatch = useDispatch(); |
There was a problem hiding this comment.
This doesn't seem to work with the thunk action I'm dispatching. Throws a TS error about the argument not being the right type
There was a problem hiding this comment.
I think this might be because we aren't using configureStore with redux and are using the old, deprecated createStore. Or how we register thunks, but that action is created in our redux package so the root store should know about it
| <> | ||
| <ThemePicker /> | ||
| <Picker | ||
| label="Choose grid density" |
There was a problem hiding this comment.
Maybe a question for @dsmmcken, but should this density apply to other Spectrum components such as ListView by default as well? https://react-spectrum.adobe.com/react-spectrum/ListView.html#density
Though some items (like Tabs) don't have a spacious option.
There was a problem hiding this comment.
One vote for no is users of dh.ui should already be able to specify density on those components.
In our UI, we would then need to test anywhere we use list or tabs (or others w/ density) looks ok w/ all density options
There was a problem hiding this comment.
That's a good point, but I would limit this ticket to just applying to grid. Maybe lists, not sure if much else has a density prop.
There was a problem hiding this comment.
Yup limit the scope of this ticket, but could be interesting to do later.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2151 +/- ##
==========================================
+ Coverage 46.62% 46.67% +0.05%
==========================================
Files 685 692 +7
Lines 38493 38622 +129
Branches 9589 9628 +39
==========================================
+ Hits 17948 18028 +80
- Misses 20535 20583 +48
- Partials 10 11 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
We should have a follow up ticket to read the default density from the server. |
3 participants
Fixes #885
This adds a user setting for grid density in the theme section which applies to all grids without an explicit density prop. A separate dh.ui PR will allow setting the density per table and is not influenced by the global density setting.