carbon-design-system / ibm-products

A Carbon-powered React component library for IBM Products
https://ibm-products.carbondesignsystem.com
Apache License 2.0
92 stars 137 forks source link

Datagrid design review: Horizontal scrolling and responsiveness #2509

Closed elycheea closed 11 months ago

elycheea commented 1 year ago

Design review


Standards

Pattern and behavior

Storybook

matthewgallo commented 1 year ago

• Rename Sticky column story to Frozen columns • Width of sticky actions column on right is too wide (width should be 48px, in addition column header should also be 48px) • There should not be any white space/margins at end of scrollable column header row • Frozen column border should use $active-light-ui • Investigate control for multi-wrap column property, multiLineWrap, also potential for adding it once for all columns and not on a per column basis • Unfreeze first column at <672px, double check frozen column header not sticking anymore

To think about: • Row index labels to left of selectable row/checkbox

elycheea commented 1 year ago

^ FYI @Ratheeshrajan

jjennings7 commented 1 year ago

@matthewgallo @elycheea @Ratheeshrajan I'm unable to check the boxes in the design review section for some reason. Can one of you please check them off for me? With the exception of the following, all the rest can be marked:

  • [ ] The component behaves according to the guidelines set by the pattern maintainer

See Matt's comment above with the few small changes needed

  • [ ] The Storybook displays multiple scenarios that show how the component can be used

There are no controls or knobs for this component. As we mentioned to Matt, it would be nice to include a control or knob so the storybook user can freeze or unfreeze the first column, or the last column (actions), or both. Currently, both are frozen in the example.

Regarding @matthewgallo 's comment above:

To think about: • Row index labels to left of selectable row/checkbox

Here's an example screenshot of what he's referring to from my product, DataStage:

Screen Shot 2022-12-15 at 3 19 00 PM

One last thing I noticed is the title on the table, "click action menu". I don't think that's supposed to be there?

Screen Shot 2022-12-15 at 3 21 38 PM

elycheea commented 1 year ago

@jjennings7 We left off the row index changes for now since it caused some unexpected behaviors and felt like it’s out of scope for our first release. We can open a separate issue as an enhancement later. If the current changes look good, feel free to close this out. 🙆‍♀️

jjennings7 commented 1 year ago

Sorry for the delay. Everything looks good and I think we can close this, just 2 notes for future:

There are no controls or knobs for this component. As we mentioned to Matt, it would be nice to include a control or knob so the storybook user can freeze or unfreeze the first column, or the last column (actions), or both. Currently, both are frozen in the example.

jjennings7 commented 1 year ago

I don't have the option to close it for some reason, @elycheea can you please close?

elycheea commented 1 year ago

Thank you @jjennings7!!

elycheea commented 1 year ago

@jjennings7 or @vsnichols just reopened so we can double check the behavior in v2 and check for accessibility.

elycheea commented 1 year ago

@kristastarr to review for accessibility. Feel free to remove any checks that don’t apply.


Accessibility review

Keyboard

Level 1

Level 2

Level 3

Dynamic updates

Level 1

Level 2

Level 3

Text and non-text

Level 1

Level 2

Level 3

User input

Level 1

Level 2

Level 3

kristastarr commented 1 year ago

Styling comments:

  1. When resizing columns such that the text is truncated in one or more of the columns, on hover, the text in both columns shows and is overlapping

    Screenshot 2023-07-05 at 10 21 01 AM
  2. When hovering the mouse on the table header text, the cursor appears as a text cursor which makes it look editable. The title text does show on hover of the header text, but not on hover of the rest of the cell. Is that the desired behavior?

  3. I re-sized the table in various ways and also used browser zoom to simulate a screen enlarger. I didn't investigate exactly when/why but sometimes there is an empty column between the headers, and sometimes the overflow menu column is the incorrect width, as shown here at 200% browser zoom:

    Screenshot 2023-07-05 at 2 10 57 PM
  4. The L/R arrow keys correctly scroll the parts of the table that are not sticky. However, depending on the width of the table, it's possible that it isn't evident that there are more columns than what are shown. To me it's not really clear from a visual standpoint that the table is scrollable in those cases. Example: here, it's not evident that there are 7 more columns in the table

    Screenshot 2023-07-05 at 11 23 55 AM

Keyboard and Mac Voiceover testing:

  1. As discussed in other issues (like Sort https://github.com/carbon-design-system/ibm-products/issues/3118) we have the situation where the contents of each cell are a group, so you have to arrow three times to move to the next cell. In this component, this behavior is present in table header and all table items here. In the Sort component, it is not present in the table header but is there in the table items.

  2. Also documented in Sort https://github.com/carbon-design-system/ibm-products/issues/3118, the row splitters are not interactable via keyboard focus, but are interactable via screen reader when navigating using CNTRL+Option+ L/R/Up/Down arrows. But even though you have the option to interact with them, they do not increase or decrease as desired and do not resize the column.

  3. divider1 divider2

  4. In this component as well as Batch actions, https://github.com/carbon-design-system/ibm-products/issues/2607 when a checkbox is selected, voiceover gives no indication that the blue "X items selected" bar appears and offers additional actions. Could look at using aria-live to provide live updates that new content has been added to the screen

Screenshot 2023-07-05 at 2 51 08 PM
jjennings7 commented 1 year ago

In addition to the styling issues Krista noted:

The tooltip for the action button seems to be within the table and not above it, so it gets cut off but you can scroll to see it, causing a white column to appear:

Image

Image

The vertical border to delineate the frozen columns seems to be missing in the header, so the header text looks cut off if it's partially scrolled:

Image

Image

Regarding the scroll bar issue Krista noted, not sure if it would exactly help that issue, but I noticed the scroll bar spans the entire width of the table instead of being contained within the scrollable area. Could/should it be contained?

Image

And lastly, I would still like to see controls or knobs so that people checking out the component know they have a couple choices of which columns to freeze and they can try it out in the storybook.

davidmenendez commented 11 months ago

after reviewing the most up to date version of datagrid it appears that all of these concerns have been addressed with the exception of this note

And lastly, I would still like to see controls or knobs so that people checking out the component know they have a couple choices of which columns to freeze and they can try it out in the storybook.

we've had some initial conversations about providing more sandboxes or codepens with datagrid examples so that users are able to interact more with the code. unfortunately, due to the nature of the mechanisms that datagrid is built on, storybook controls aren't well suited to allow users to turn things on and off.

closing this as complete 🥳