Skip to content
This repository has been archived by the owner on Oct 19, 2021. It is now read-only.

feat: Table toolbar v10 #2247

Merged
merged 26 commits into from
Apr 23, 2019
Merged

Conversation

vpicone
Copy link
Contributor

@vpicone vpicone commented Apr 19, 2019

Closes IBM/carbon-components-react#2166

Add support for new v10 features

TODO:

  • Update markup
  • Update classnames
  • Create menu component
  • Update action component
  • TableToolbarSearch expansion needs should be a controlled/uncontrolled component
  • Search filtering
  • Batch Actions
  • Update stories for new markup
  • Switch toolbar for menu in all stories
  • add new table modifiers and remove old ones
  • add knobs to stories
  • focus search input when expanded
  • adjust default story and prop naming

Changelog

New

  • TableToolbarMenu
  • Table modifiers
  • Story knobs

Changed

  • Actions are now overflow menu items that live within a single menu
  • Slight changes in markup requiring data table

Removed

  • Deprecation warnings

@netlify
Copy link

netlify bot commented Apr 19, 2019

Deploy preview for carbon-components-react ready!

Built with commit 7fc03fa

https://deploy-preview-2247--carbon-components-react.netlify.com

@vpicone vpicone requested a review from joshblack April 19, 2019 19:02
This commit adds all of the new modifiers and removes the unusued ones.
All of the datatable stories have been updated to reflect new markup
and to expose knobs to adjust shared modifiers.

The dynamic content story was updated to use the new menu system. This
also adds support for styles that have not been added yet (row size).
@vpicone vpicone marked this pull request as ready for review April 22, 2019 01:58
Copy link
Contributor

@asudoh asudoh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 LGTM basically, just a couple of comments. Thanks @vpicone!

src/components/DataTable/TableToolbarMenu.js Show resolved Hide resolved
src/components/DataTable/TableToolbarSearch.js Outdated Show resolved Hide resolved
@vpicone
Copy link
Contributor Author

vpicone commented Apr 22, 2019

@asudoh fixed thank you!

@asudoh
Copy link
Contributor

asudoh commented Apr 22, 2019

Thanks @vpicone for your quick update! Would you update the snapshot?

@vpicone vpicone requested review from dakahn and removed request for joshblack April 22, 2019 12:43
@vpicone
Copy link
Contributor Author

vpicone commented Apr 22, 2019

@dakahn The TableToolBarSearch is a weird component (button-div encapsulating/hiding an input). I'm happy with the keyboard navigation but definitely open to an a11y insight. I played with having the div be a button, but styling get really wild with an input nested in a button. As a result, I needed to add a tabindex="0" to make the div tabable and direct focus to the input.

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vpicone Thanks for the ping! 🏄🏽‍♂️

Testing in latest Chrome on macOS, and Windows 10 on IE11/latest Chrome are five points of interest:

  • inadequate/inconsistent focus state on dropdown menu button on click
  • when dropdown is activated via keyboard the first menuItem should receive focus
    Apr-22-2019 09-44-22
  • there is no grouping element telling a user they're currently interacting with a landmarked group data table toolbar. Making skipping to table content/returning to toolbar frustrating (in the example the unordered list is our sidebar)
    Apr-22-2019 09-47-59
  • when the user inputs a filter/search there is no feedback indicating the table has been updated as a live region
    Apr-22-2019 09-47-07
  • Funny lookin' business on IE11. Resizing DataTable and cutoff search bar focus style
    Apr-22-2019 09-54-32

vpicone and others added 4 commits April 22, 2019 10:59
This updates the stories so that they only use the feature they are
showcasing. This will prevent users from bringing in DataTable features
that they don't need.

A "kitchen sink" data table might be useful as well.
@vpicone
Copy link
Contributor Author

vpicone commented Apr 22, 2019

  • there is no grouping element telling a user they're currently interacting with a landmarked group data table toolbar. Making skipping to table content/returning to toolbar frustrating (in the example the unordered list is our sidebar)
  • when the user inputs a filter/search there is no feedback indicating the table has been updated as a live region
  • inadequate/inconsistent focus state on dropdown menu button on click
  • when dropdown is activated via keyboard the first menuItem should receive focus
  • Funny lookin' business on IE11. Resizing DataTable and cutoff search bar focus style

@dakahn I'm going to make a new issue for the first two incomplete tasks since I believe they're inherent to the overflow menu itself and might require breaking changes.

If you don't mind making an issue for the ie11 one, I don't have ie11 going atm so can't reproduce or debug

@vpicone vpicone requested a review from dakahn April 22, 2019 19:23
@jnm2377
Copy link

jnm2377 commented Apr 22, 2019

It's looking really good! I noticed the IE11 bug that @dakahn was talking about is also happening in Chrome when you type in the search bar.
toolbar

@vpicone
Copy link
Contributor Author

vpicone commented Apr 22, 2019

@jnm2377 the size of the table is based off of the wrapper width and the contents. Since we don't define a wrapper here, the table is only stretched by the width of its rows. When there's no content, there's no width (up until the min-width)

Search isn't working in vanilla, but if you delete the tr you can see the same thing happening here: https://www.carbondesignsystem.com/components/data-table/code

src/components/DataTable/TableToolbar.js Outdated Show resolved Hide resolved
dakahn and others added 3 commits April 23, 2019 09:04
Received the following linting error when attempting to define tabindex
on the section component:

`tabIndex` should only be declared on interactive
elements.eslint(jsx-a11y/no-noninteractive-tabindex)
Copy link

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Toolbar is working as expected 👍 I think if there are specific a11y concerns regarding other components used for the toolbar, it would make sense to open a separate PR fixing that specific component just so that we can keep the scope of this PR to just data table.

@vpicone
Copy link
Contributor Author

vpicone commented Apr 23, 2019

LORD SAVE ME FROM THESE SNAPSHOTS

@vpicone vpicone changed the title Table toolbar v10 feat: Table toolbar v10 Apr 23, 2019
@vpicone vpicone merged commit 01360a6 into carbon-design-system:master Apr 23, 2019
@vpicone vpicone mentioned this pull request Apr 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data table rendering without styles
4 participants