DataViews: API for layout density support #66837
Conversation
ntsekouras
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @annchichi. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
packages/components/src/toggle-group-control/toggle-group-control/component.tsx
Outdated
Show resolved
Hide resolved
| @@ -30,6 +30,7 @@ export const VIEW_LAYOUTS = [ | |||
| label: __( 'Grid' ), | |||
| component: ViewGrid, | |||
| icon: category, | |||
| supportsDensity: true, | |||
There was a problem hiding this comment.
This could also be an array of supported presets.
There was a problem hiding this comment.
We already have some hardcode logic in the view config that is layout specific. Note how the arrows to "move fields right/left" only show up in the table layout is active but not in any other layout:
This is hardcoded logic we should aim to remove. The view config components should be layout agnostic, otherwise, we cannot open it to 3rd party layouts.
Connecting that thought with what you're doing here, it may be a good time to design this API to "declare features supported by the layout" instead of making it specific to density. For example, using supports: [ 'layout' ] or supports: { layout: true } instead of supportsDensity: true.
There was a problem hiding this comment.
I agree and was something I had in mind to do (your suggestion) if we had another case in the future. Since we have, I'll do it now. Thanks!
ntsekouras
requested review from
mcsf,
youknowriad,
oandregal,
jameskoster and
jasmussen
|
Flaky tests detected in 9ee1bbb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11911243411
|
This sounds a little strange to me. In terms of UX, modifying the density preference should always result in a visual change. The tricky part here is aligning the three options to configurations in grid layout, accounting for container sizes. For instance we might say:
And that might work well in most cases. However, on small (mobile) screens the Pages grid would be very cramped at 5 columns, and 3 columns probably doesn't qualify as 'comfortable' :). A single-column layout seems legitimate at smaller sizes, and this wouldn't be possible. Moreover on a super wide screen you might want more than 5 columns. So I suppose the question to me is whether it should be possible for:
Then we can do something like:
I quite like the idea of using |
I also like the and ideally, we can make it work :) but if the localisation brakes it I also like the idea of the RadioControl. To be fair I was thinking about the radio buttons here, but thought they weren't allowed in that panel so used this instead :D |
This implementation uses dynamic min/max values based on viewport.
I'm not sure this will entirely solve the problem as a user could have selected a density option and then opens the menu and sees a different one selected? Having 3 options always seem consistent and the downside could be probably in mobile that comfortable and medium having the same value (1 column). I think that could be fine personally. In the other cases, since we have control over the For reference these are the default columns ( |
packages/dataviews/src/dataviews-layouts/grid/use-density-options.ts
Outdated
Show resolved
Hide resolved
| const isXHuge = useViewportMatch( 'xhuge' ); | ||
| const isHuge = useViewportMatch( 'huge' ); | ||
| const isXlarge = useViewportMatch( 'xlarge' ); | ||
| const isLarge = useViewportMatch( 'large' ); |
There was a problem hiding this comment.
Tangential to the API discussion, but this may warrant creating a more appropriate alternative to calling useViewportMatch four times, no? I say this because it's calling useMediaQuery each time, which seems expensive.
There was a problem hiding this comment.
Yeah, I simplified a bit from the existing version, but will try to see if I can avoid these calls.
|
The way we've been going about these is that any layout specific property belongs in |
ntsekouras
force-pushed
the
dataviews/add-density-support-for-layouts
branch
from
af17dba to
0087c4f
Compare
Took this PR for a spin. In this branch, the density options don't update upon viewport changes, while in
Screen.Recording.2024-11-13.at.10.10.47.movThis branch: Screen.Recording.2024-11-13.at.10.12.33.mov |
|
I rebased and addressed most of the current feedback. Regarding the columns per viewport I changed the breakpoints to be: This results in visual change with two exceptions, which are probably ok in my opinion. In mobile: In small: I'd argue that in such small viewports users wouldn't bother much in |
|
@oandregal I just pushed some changes, so it would be good to test again. The main difference from trunk is that we had this specific density implementation for I'm not sure it's better to add/remove options dynamically in an API that in most layouts the density should have visible results in any viewport. Layouts like grid are just a bit more nuanced, because they work better by knowing and using as best they can the viewport width. |
| /** | ||
| * The density of the view. | ||
| */ | ||
| density?: Density; |
There was a problem hiding this comment.
API-wise, I think this is the right choice.
A DataViews component prop seems a bit detached from what we already have (view config, default layouts).
view vs view.layout: density is a tool that DataViews shows to users in the view config. The view.layout is about things that are layout-specific and that the rest of the DataViews component doesn't know anything about (e.g.: the logic to handle the media field of grid is contained within the layout itself, there's no other controls in DataViews affected by that).
|
Nice work, this seems to work well enough for me. At one of the densities, I did see non-square thumbnails at this viewport size, though: As for the slider itself, "Medium" is an odd one out, as a t-shirt size in between adjectives. Could we go with what Gmail does, Default/Comfortable/Compact instead? Or some other sequences?
The segmented control is also hugely prominent and probably doesn't need to be a segmented control. Can we do dropdowns instead? 
|
ntsekouras
force-pushed
the
dataviews/add-density-support-for-layouts
branch
from
d94a91a to
e83e6ea
Compare
|
I updated the |
ntsekouras
force-pushed
the
dataviews/add-density-support-for-layouts
branch
from
e83e6ea to
5c4e4cf
Compare
|
Size Change: -133 B (-0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
My preference would be to use
This part still feels a bit strange to me, but I suppose we can investigate in a follow-up based on feedback. |
Thanks, I think it works well @ntsekouras |
ntsekouras
force-pushed
the
dataviews/add-density-support-for-layouts
branch
from
5c4e4cf to
9ee1bbb
Compare
|
@jameskoster I brought back the |
ntsekouras
force-pushed
the
dataviews/add-density-support-for-layouts
branch
from
9ee1bbb to
764a09b
Compare
There was a problem hiding this comment.
Code-wise this is looking good. I only have one suggestion for the API. I'll do a final review after design approval in case things need to change.
|
@ntsekouras @jameskoster is there any way to see it localised to other language variants? I think this was the only concern, but I'll leave it to @jameskoster to confirm |
|
I'm closing in favor of: #67170. So, please share feedback there. Thanks for all the help here! After further consideration an abstraction for the density concept isn't a good fit for layouts at this moment. I explain more in that PR's description. |
What?
Part of: #66312
In #63367 a density picker was introduced for
gridlayout specifically. Density though is a concept that any layout could support, so we need an API for that.The goals for this PR are:
grid. The idea is to have three density presets (comfortable, balanced, compact) for a user to choose from. A layout should have a flag if it supports density (it could also be possible to make the API to support specific density options and not all three of them..) and would handle the styles for each preset. In my initial commit I added thedensityprop to theviewobject, but it could also be underview.layout- I have to think more about it.comfortable, balanced, compactpresets.gridlayout should support density in a good, acceptable way. The main difference from trunk is that based on the viewport width we would show fewer suggestion in some cases, but here we always show three options. With the current values I'm using, this essentially results in having the same grid columns in two presets, at some viewport sizes. This can be discussed more though and/or adjust the number of columns. Noting that this shouldn't be the case for layouts liketable or list, as they expand vertically.Tasks
Follow ups
tablelayout (resolve Data views: Add density preference to Table layout #66312)Testing Instructions
Screenshots or screencast
Screen.Recording.2024-11-07.at.7.40.57.PM.mov