Graduate data view options out of a menu to allow more design expression #64175
Conversation
|
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 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. |
|
Size Change: +1.03 kB (+0.06%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
Great start. I've been testing this and it works well.
|
| font-size: $helptext-font-size; | ||
| font-style: normal; | ||
| color: $gray-700; | ||
| } |
There was a problem hiding this comment.
I'm a bit surprised by the number of styles that we need to make this look correct. Do we need all these "widths", "margins", "paddings"... Why our default components don't work well? What's missing as a UI component to render that dropdown?
There was a problem hiding this comment.
Reading the code again, it seems mostly about the SettingsSection component. I would suggest extract this component to its own folder dataviews/src/components/fieldset or something to start with and IMO, this should be a UI component like a Fieldset component or something. I also wonder if it should be using a fieldset + legend element (not entirely sure)
There was a problem hiding this comment.
We could be able to save some of these styles by using the Text component.
|
Nice work Jorge, this is working really well already imo.
Yes, I don't think we should remove the top-level layout switcher. The thinking behind including it in the popover is that it would give context to some of the layout-specific options that appear conditionally, like grid density. However seeing it in practise the jump when switching to list layout is a bit extreme. Let's leave the layout switcher as it is for now.
This isn't on trunk, so probably fine to add separately? If we do, we should update the description so it doesn't mention re-ordering :) @jorgefilipecosta instead of toggling the opacity on the visibility button, could we toggle the icon instead (between The spacing in the I'd appreciate feedback from @WordPress/gutenberg-design on the idea of referring to fields as 'properties'. It feels more user-friendly to me, especially with one eye on content modelling. |
|
I pushed a couple of minor style adjustments. |
|
@jorgefilipecosta one small styling discrepancy I couldn't figure out... the grid seems to be correct, but the dimensions are off. Notice that the inputs are narrower than the title + description section: 
Edit: It seems to be caused by https://github.com/WordPress/gutenberg/pull/64175/files#diff-852fc67313b211cde3af980172ba9c5410fda0faaf11128fe3990a3afb0445b6R34-R36. I'll look into it next week unless you get there first :) |
|
Hi @jameskoster, good catch. |
|
Maybe we can use css sub grid? Here's a codepen that seems to work. 
|
jorgefilipecosta
force-pushed
the
update/new-view-config-UI
branch
from
933a3cc
to
37d6f0e
Compare
There was a problem hiding this comment.
I've left some drive-by comments as I was responding to a specific ping.
The spacing in the
ItemGroupis a bit clunky. It's not something to address here, but in the component. I know it's on @WordPress/gutenberg-components' list, but wanted to note another occurrence.
@jameskoster could you please elaborate on the spacing issues you're seeing? Yes, we've been meaning to do some unification of the menu and item components (see #35210) plus some general flexibility and integration improvements to ItemGroup (see #34709) but I'm not sure we're aware of any spacing issues, specifically.
packages/dataviews/src/components/dataviews-view-config/index.tsx
Outdated
Show resolved
Hide resolved
| ...defaultLayouts[ newLayout ], | ||
| } ); | ||
| } | ||
| throw new Error( 'Invalid dataview' ); |
There was a problem hiding this comment.
Do we really want to throw errors in an onChange handler? I'd expect a warning() or something less invasive. I see this was here before, so just something to keep in mind.
There was a problem hiding this comment.
Good point, I followed what we had without thinking much but a warning seems like a better option. The code was updated 👍
packages/dataviews/src/constants.ts
Outdated
| @@ -58,6 +58,10 @@ export const sortLabels = { | |||
| asc: __( 'Sort ascending' ), | |||
| desc: __( 'Sort descending' ), | |||
| }; | |||
| export const sortLabelsShort = { | |||
| asc: __( 'ASC' ), | |||
There was a problem hiding this comment.
AFAIK we rarely capitalize strings; instead, capitalization is achieved cosmetically with CSS.
Also, note that this can be problematic for translation in some languages. If we can, I'd recommend we use the full words (ascending / descending)
There was a problem hiding this comment.
I updated the code to use CSS as suggested. Regarding using full words (ascending / descending), in this case, it would break the design:
I added a translation context, that also specifies a character limit for the short labels.
There was a problem hiding this comment.
@jorgefilipecosta thanks for the screenshot. This is exactly what will happen with the translations, if folks start translating and there's no good abbreviation in their language.
I wonder if a worthy alternative is to use icons instead of ASC/DESC text labels. We have a few up/down arrow icons available that we could use here. @jameskoster WDYT?
There was a problem hiding this comment.
Yup, let's try an icon :)
| </DropdownMenu> | ||
| <Grid columns={ 2 } className="dataviews-settings-section" gap={ 4 }> | ||
| <VStack spacing={ 0 }> | ||
| <h2 className="dataviews-settings-section__title">{ title }</h2> |
There was a problem hiding this comment.
Maybe we can use the __experimentalHeading component for these headings? Could perhaps save some custom styles.
There was a problem hiding this comment.
The code was updated and indeed it allows us to remove some custom css.
| font-size: $helptext-font-size; | ||
| font-style: normal; | ||
| color: $gray-700; | ||
| } |
There was a problem hiding this comment.
We could be able to save some of these styles by using the Text component.
@tyxla it relates mostly to the dimensions. Most interactive elements follow a 24/32/40px sizing scale for height, but due to the unconventional padding on |
Got it. cc @mirka who has been working on the transition for the 40px component sizes. |
jorgefilipecosta
force-pushed
the
update/new-view-config-UI
branch
2 times, most recently
from
b6dd8ca
to
65fc176
Compare
Thank you a lot for the grid sample provided @jameskoster. I updated the code to use something similar to what you suggested and the dimensions issue you noticed is now fixed. |
|
Thank you for the review @tyxla, I have addressed the feedback provided. |
jorgefilipecosta
force-pushed
the
update/new-view-config-UI
branch
from
74b37b6
to
171b00c
Compare
Hi @youknowriad, the PR was updated and the layout type was left as is. I will prioritize working on reordering and grid density in the popover as follow-up tasks. The main objective of this PR is to simply implement the popover with the existing functionality of the menu without making it too extensive. |
|
Hi @jameskoster,
I made that update, I think this PR should be ready for another look. |
| __nextHasNoMarginBottom | ||
| __next40pxDefaultSize | ||
| isBlock | ||
| label={ __( 'Order' ) } |
There was a problem hiding this comment.
If I click the "sort by" label, the select is focused properly, so the label is working properly but it's not the case for "Order" or "Items per page". I expect the button group (or the first button) to be focused. It seems we might have an issue in the component. cc @tyxla
There was a problem hiding this comment.
I have thought about this, but I'm not sure on what basis we would want to implement that behavior. Usually we do it for accessibility (i.e. increase the click target on things like checkboxes and radios, or because "the browser does it by default" when an input is properly associated with a label). In the case of compound components like ToggleGroupControl, I don't think there's a click target issue, and since in normal HTML semantics the label would actually be associated with a radiogroup/group, or be a legend in a fieldset, that kind of click-to-focus behavior would not happen.
So the only reason to add click-label-to-focus on compound components would be for UX reasons (consistency), which may possibly be bad for accessibility due to being non-standard 🤔
There was a problem hiding this comment.
The behavior here is expected AFAIK, because the underlyingToggleGroupControl behaves like a radiogroup with radio buttons. You can see an example with actual radio buttons here: https://ariakit.org/components/radio where you can tab into and out of the component with Tab / Shift + Tab, and you can use the arrows to select a different radio button.
If you're referencing the focus styles, this is another thing: we've been debating this in #63450 (comment) and decided to not move the focus to the ToggleGroupControlOption because, it a nutshell, it would look odd with the selected style. We can always re-evaluate that if you feel like it was not the best path forward.
There was a problem hiding this comment.
So the only reason to add click-label-to-focus on compound components would be for UX reasons (consistency), which may possibly be bad for accessibility due to being non-standard 🤔
Yes, that's my point for a user, it's a single control, just like the others in the form. It's not really clear why there's a different between these controls and the "sort by" for instance.
I can live with that for now, but for me it's still a bug and there should be a way to fix the UX without breaking the a11y IMO.
|
One small change; we can probably leave out the descriptions for now. I tried to remove them but got an error. To be clear; in |
| @@ -0,0 +1,43 @@ | |||
| .dataviews-view-config { | |||
| width: 363px; | |||
There was a problem hiding this comment.
Let's make this a bit narrower for now, and align to the 4px baseline. 320px works well.
|
|
||
| .dataviews-settings-section__title.dataviews-settings-section__title { | ||
| line-height: $grid-unit-30; | ||
| font-size: 15px; |
There was a problem hiding this comment.
Not something to address here, but it would be great to update the Text / Heading components to support type styles so that we can avoid this kind of css. cc @tyxla.
There was a problem hiding this comment.
I believe both the Text and the Heading components accept a size prop that you can use:
<Heading size={ 18 } />My heading</Heading>
<Text size={ 13 } />My text</Text>
Now, I can argue whether doing this is a good idea. Specifying an arbitrary size can work against our efforts to bring consistency across the editor. Ideally, you would use one of the available level values of Heading (1 to 6), and Text would be consistent across the editor.
cc @WordPress/gutenberg-components in case anyone else has further thoughts.
There was a problem hiding this comment.
Yup, ideally we move away from arbitrary sizes entirely. I know it's no small task though.
jorgefilipecosta
force-pushed
the
update/new-view-config-UI
branch
from
87230af
to
006fb8c
Compare
jorgefilipecosta
added
the
[Feature] Data Views
| onClick={ () => setIsShowingViewPopover( true ) } | ||
| /> | ||
| { isShowingViewPopover && ( | ||
| <Popover |
There was a problem hiding this comment.
Do you know why we're using Popover (too low level) here, instead of Dropdown ?
The reason I'm asking is that I see a lot of differences between this dropdown and the "layout type dropdown"
- The spacing is not the same between the anchor and the popover
- The behavior is not the same: one dropdown disabled everything behind it (inhert, disables scrolling...) and the other doesn't
This PR implements the new view config UI proposed at #63872.
To keep this PR manageable and not very big we just port the existing functionality of the old dropdown menu. (Change some parameters, and hide/show fields).
Things like moving the item size picker for the grid into the popover, or sort directly on the field list will come as follow-ups.
This PR tries to implement the patterns specified in #63624.
SettingsSection has container queries to make the component responsive depending on the size of the container.
Screenshots
cc: @jameskoster
Testing Instructions
I verified I could configure all the fields and things worked as expected.
Using the browser inspector I added the following CSS rule:
I verified I got the responsive behavior shown above.