Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Datagrid design review: Toolbar #3117

Closed
2 tasks done
Tracked by #2537 ...
elycheea opened this issue Jun 9, 2023 · 9 comments
Closed
2 tasks done
Tracked by #2537 ...

Datagrid design review: Toolbar #3117

elycheea opened this issue Jun 9, 2023 · 9 comments
Assignees
Labels

Comments

@elycheea
Copy link
Contributor

elycheea commented Jun 9, 2023

Design review


Standards

  • All pattern updates/changes/iterations have been discussed with the
    component developer
  • Patterns have been approved by either DSAG or another approving entity

Pattern and behavior

  • The component behaves according to the guidelines set by the pattern
    maintainer
  • The component’s UI matches the pattern specifications set by the pattern
    maintainer
  • The component’s motion matches the specifications set by the pattern
    maintainer(s)
  • The UI produced is responsive, cross-browser, and responds to the
    currently set Carbon theme.
  • Colors, themes, sizes and additional props are true and correct, aligning
    with Carbon set tokens (unless otherwise specified by the pattern)
  • Paddings/margins/spacings are true and correct, in both desktop and mobile
    views

Storybook

  • A functioning component renders in Storybook
  • The Storybook displays multiple scenarios that show how the component can
    be used
  • Changing props in the Storybook updates the component

Tasks

Preview Give feedback
  1. Sev 3
    davidmenendez
  2. 0 of 4
    component: Datagrid
    davidmenendez
@elycheea elycheea added component: Datagrid status: needs design review 🎨 Component is ready for design review labels Jun 9, 2023
@elycheea elycheea moved this to Design review 🎨 in Carbon for IBM Products Jun 9, 2023
@elycheea elycheea moved this from Design review 🎨 to Needs review 👋 in Carbon for IBM Products Jun 9, 2023
@elycheea elycheea changed the title Datagrid design review: toolbar Datagrid design review: Toolbar Jun 9, 2023
@elycheea elycheea added version: 2 Carbon 11 / v2 version: 1 Carbon 10 / v1 labels Jun 9, 2023
@elycheea
Copy link
Contributor Author

@Ashley-Bock can you take this one?

@elycheea
Copy link
Contributor Author

elycheea commented Jun 12, 2023

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


Accessibility review

Keyboard

Level 1 - see comments for items needing further attention

  • Properly code the navigation order
    • Ensure interactive elements are reachable with the keyboard
    • Where possible, achieve the desired tab order by adjusting the DOM,
      rather than overriding the tabindex
    • When overriding the default focus indicator, confirm focus is highly
      visible
  • Implement established keyboard conventions
    • Use standard HTML elements where possible, using CSS to alter appearance
      not behavior
    • Be familiar with established keyboard conventions
    • Implement keyboard operation for custom elements

Level 2

  • Ensure keyboard access to complex interactions - see comments

Dynamic updates

Level 2

  • Make content accessible that appears on hover or focus
    • Ensure Esc dismisses new content - see comments
    • If content appears on hover, the new content needs to remain visible
      until dismissed
    • Moving the pointer away from the trigger should not be the action that
      dismisses the new content
    • Ensure custom tooltips and similar hover text can be triggered by
      keyboard
    • Discuss alternatives to hover text
    • Use caution where interactive controls appear in the hover text
    • Provide accessible ways of cancelling pointer interaction

Text and non-text

Level 3

  • Order content appropriately in the DOM - see comments about tabbing from Primary button to Create new

@kristastarr kristastarr self-assigned this Jun 13, 2023
@Ashley-Bock
Copy link

1. Pop-out styling
There are discrepancies between v1 and v2 in terms of the pop-out styling. The v1 better matches the specs in the PAL guidance images, but I'm not sure if the change in v2 was intentional. See below:

v2:
Screenshot 2023-06-14 at 3 17 50 PM

v1:
Screenshot 2023-06-14 at 3 18 26 PM

  • Action item: Determine if pop-out change was intentional and unify visuals in PAL guidance to match v2 specs.

2. Order of sizes listed
In the examples in the PAL guidance, the sizes are listed smallest to largest. In both v1 and v2, they are listed largest to smallest.

v2 example:
Screenshot 2023-06-14 at 3 21 25 PM

PAL guidance
image

  • Action item: decide which order to display sizes and unify visuals in PAL guidance.

3. Toolbar height
In the guidance, it says that the toolbar height should change as the user adjusts the row height. Neither v1 or v2 demonstrate this in the storybook.

  • Action item: Decide if this should still be included in the pattern even if it is not going to be supported behavior of the component.

@Ashley-Bock
Copy link

Ashley-Bock commented Jun 14, 2023

Standards

  • All pattern updates/changes/iterations have been discussed with the component developer
  • Patterns have been approved by either DSAG or another approving entity

Pattern and behavior

  • The component behaves according to the guidelines set by the pattern maintainer
  • The component’s UI matches the pattern specifications set by the pattern maintainer
  • The component’s motion matches the specifications set by the pattern maintainer(s)
  • The UI produced is responsive, cross-browser, and responds to the currently set Carbon theme.
  • Colors, themes, sizes and additional props are true and correct, aligning with Carbon set tokens (unless otherwise specified by the pattern)
  • Paddings/margins/spacings are true and correct, in both desktop and mobile views

Storybook

  • A functioning component renders in Storybook
  • The Storybook displays multiple scenarios that show how the component can be used
  • Changing props in the Storybook updates the component

@elycheea
Copy link
Contributor Author

  1. Pop-out styling
    @matthewgallo I think we changed this to Toggletip to avoid the redundant tooltip. fix(Datagrid): tooltip placement change for row height settings #2873 Since our product teams provide their own DatagridActions, it might be good to have a good recommendation on Toggletip, OverflowMenu, or other Popover styles? 🤔 @Ashley-Bock I think this one is specific to the row size design review, but for the consistency of the Toolbar, may be a good topic to bring up? v2/Carbon 11 allows use of the newer Toggletip and Popover components, but v1/Carbon 10 only have OverflowMenu.

  2. Order of sizes
    I think this one is actually configurable by the users of the component so I think this is okay?

  3. Toolbar height
    @matthewgallo is this something we changed intentionally, something users have control over instead of us, or something that was missed. I could imagine this being a little bit tricky for some of the smaller sizes.

@elycheea elycheea moved this from Needs review 👋 to Design review 🎨 in Carbon for IBM Products Jun 16, 2023
@elycheea elycheea removed the status: needs design review 🎨 Component is ready for design review label Jun 16, 2023
@elycheea
Copy link
Contributor Author

@Ashley-Bock @matthewgallo So I went looking to figure out whether Toggletip or Popover makes more sense in this context. Then I stumbled upon #2527 which @AlexanderMelox and I happened to find a while back.

I think anything that drops down from the toolbar should be considered a “disclosure1.

Settings and filters follow the disclosure pattern by clicking an icon button that opens a popover. Interactive elements are often found in settings and filters in order to modify or adjust specific content. The disclosure pattern allows you to nest interactive elements inside of a popover while still being accessible.

So based on this, I think the original design with the Popover is most likely the way to go for now. @Ashley-Bock not sure if we need to add some context/guidance on the disclosure pattern in the PAL guidance, but I don’t think we have a dedicated Toolbar page anyway.

Footnotes

  1. Disclosures via Carbon Design System.

@kristastarr
Copy link
Contributor

kristastarr commented Jul 3, 2023

Some comments about keyboard accessibility - mostly related to Settings. I tried to capture in screen recording.

  1. In most cases, when you tab from Download button to Settings button, you are able to press space/enter to open the Settings popover correctly. However, sometimes when you tab to it, only a faint 1px blue outline appears around the button (which does not match the rest of the focus states), and then nothing happens when you press space, and focus is lost. After pressing tab twice, focus resumes on the settings button. I’ve tried to determine what pattern of actions causes it to work vs not work. I think it might be alternating between correct and incorrect, but I’m not 100% sure.

  2. When you are able to correctly open the dropdown, you must press tab again - and the faint 1px blue focus state appears around settings, - then press tab a second time to enter the list before you are able to press up/down/L/R to navigate the list. Focus should be present in the first list item after pressing space/enter to activate the popover. Also as you press up/down arrows to cycle through the radio button options, the table itself moves up and down behind it.

  3. After using up and down to move focus to your selected radio button, pressing tab moves you to the “Primary button” but wondering if it would be better to press enter to make a selection, which would move focus to the settings button, then use tab to move to the primary button. I’m looking for some examples of best practices to reference.

  4. When popover is open and you tab to settings, it closes. However, if popover is open and you shift+tab to Download before interacting with the popover content, the popover remains open and overlaps with the Download tooltip. I’m thinking it should close so that what happens with shift+tab is consistent with what happens with tab.
    Screenshot 2023-07-03 at 8 16 05 AM

  5. When open, the popover should dismiss when Esc is pressed

  6. When Primary button has focus, and you press tab, focus is unknown, and then if you press tab again it lands on the next interactive element (the “Create new” button), instead of going directly to the Create new button. I think the dropdown for "Primary button" is a tab stop even when it is not displayed.

  7. Visual things:
    The “Create new” text is not centered vertically in the button
    The focus state on “View documentation” looks weird since there is lots of white space on the right of the text as the focus indicator takes up the entire width of the “empty-state__content” container
    Screenshot 2023-07-03 at 8 16 23 AM

toolbar.screen.recording.mov
  1. Primary button dropdown - you can correctly tab to the primary button and press enter/space to activate the dropdown, then first option in the dropdown has focus. However, if you then shift+tab to try to return focus to the primary button, the entire dropdown menu takes focus and no interactions are available, you must press shift+tab again to return focus to the primary button.

  2. When the primary button takes focus, it looks like the blue focus ring is present for a quick millisecond before disappearing. I'm not sure if others are noticing this flicker. The tooltip itself would meet criteria for focus indicator, but I think having the outline in addition would make it even more clear. so we could maybe consider keeping the focus ring as well, especially if others think the flicker of the outline is noticeable, or find a way to remove the outline altogether so there is no flicker.
    https://github.com/carbon-design-system/ibm-products/assets/21047571/85ae6efd-8b37-43ff-bb6a-6732d3e3a877

@kristastarr
Copy link
Contributor

Tested with Mac Voiceover - everything except Settings works well. For Settings, after you click/activate the button, there is no information about the popover and it's status of open/closed, or it's contents and how to interact with them, so need to add some aria labels here.
See https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-haspopup

@elycheea
Copy link
Contributor Author

  1. Focus should be present in the first list item after pressing space/enter to activate the popover. Also as you press up/down arrows to cycle through the radio button options, the table itself moves up and down behind it.
  2. When popover is open and you tab to settings, it closes. However, if popover is open and you shift+tab to Download before interacting with the popover content, the popover remains open and overlaps with the Download tooltip. I’m thinking it should close so that what happens with shift+tab is consistent with what happens with tab.
    Screenshot 2023-07-03 at 8 16 05 AM
  3. When open, the popover should dismiss when Esc is pressed

Documented in the new issue (#3241).

  1. However, sometimes when you tab to it, only a faint 1px blue outline appears around the button (which does not match the rest of the focus states), and then nothing happens when you press space, and focus is lost. After pressing tab twice, focus resumes on the settings button. I’ve tried to determine what pattern of actions causes it to work vs not work. I think it might be alternating between correct and incorrect, but I’m not 100% sure.

I wasn’t able to replicate this. Were you using a specific browser?

  1. After using up and down to move focus to your selected radio button, pressing tab moves you to the “Primary button” but wondering if it would be better to press enter to make a selection, which would move focus to the settings button, then use tab to move to the primary button. I’m looking for some examples of best practices to reference.

No specific tasks for this one at this time but hopefully we’ll have a better idea about the approach as more people use the component.

  1. When Primary button has focus, and you press tab, focus is unknown, and then if you press tab again it lands on the next interactive element (the “Create new” button), instead of going directly to the Create new button. I think the dropdown for "Primary button" is a tab stop even when it is not displayed.

I believe this is actually the whole data table body is its own tab stop. Don’t recall if there was a specific reason for it, but open to more discussion on whether to change that behavior. (Might have implications in other stories though.)

  1. Visual things:
    The “Create new” text is not centered vertically in the button
    The focus state on “View documentation” looks weird since there is lots of white space on the right of the text as the focus indicator takes up the entire width of the “empty-state__content” container

Not an issue with the Datagrid in particular — believe this came from an update in the Carbon buttons but should be something we can address separately. I think both of these stem from the EmptyState components though.

  1. Primary button dropdown - you can correctly tab to the primary button and press enter/space to activate the dropdown, then first option in the dropdown has focus. However, if you then shift+tab to try to return focus to the primary button, the entire dropdown menu takes focus and no interactions are available, you must press shift+tab again to return focus to the primary button.
  2. When the primary button takes focus, it looks like the blue focus ring is present for a quick millisecond before disappearing. I'm not sure if others are noticing this flicker. The tooltip itself would meet criteria for focus indicator, but I think having the outline in addition would make it even more clear. so we could maybe consider keeping the focus ring as well, especially if others think the flicker of the outline is noticeable, or find a way to remove the outline altogether so there is no flicker.

Not prioritizing these for now since these will be replaced with the Carbon MenuButton.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

No branches or pull requests

3 participants