feat(DataTable): add small toolbar #5479

asudoh · 2020-02-28T08:36:46Z

This change adds small variant of table toolbar, used with compact/short variant of table rows.

There were styles implemented for that presumably as part of original v10 work, but it was not used somehow.

Fixes #5420.

Changelog

New

Small variant of table toolbar, used with compact/short variant of table rows.

Testing / Reviewing

Testing should make sure our data table is not broken.

This change adds small variant of table toolbar, used with compact/short variant of table rows. There were styles implemented for that presumably as part of original `v10` work, but it was not used somehow. Fixes carbon-design-system#5420.

netlify · 2020-02-28T08:51:54Z

Deploy preview for carbon-elements ready!

Built with commit 1b99c97

https://deploy-preview-5479--carbon-elements.netlify.com

netlify · 2020-02-28T08:53:10Z

Deploy preview for carbon-components-react ready!

Built with commit 1b99c97

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

tw15egan · 2020-02-28T13:31:03Z

packages/react/src/components/DataTable/TableToolbar.js

-    {children}
-  </section>
-);
+const TableToolbar = ({ children, size, ...rest }) => {


size seems to always be equal to normal. Do we need to update anything in the storybook examples to get this to work properly?

tw15egan

I'm unable to see any changes when changing the row size via the knob on the with toolbar story

laurenmrice · 2020-02-28T17:15:09Z

same ^

emyarod

just a heads up, snapshots will need to be updated for CI

joshblack · 2020-02-28T21:26:22Z

packages/react/src/components/DataTable/DataTable.js

+    /**
+     * `normal` Change the row height of table
+     */
+    size: PropTypes.oneOf(['compact', 'short', 'normal', 'tall']),


Would we rather this prop just live on the toolbar instead of being something passed in from DataTable with getToolbarProps?

Also, seems like the comment is related to row size changes potentially?

As the matter of fact, <DataTable> has size prop already and thus I thought it's sensible to propagate that to the tool bar. Let us know if you still like letting user set the toolbar size in application code, or something else.

@asudoh got it, makes sense!

joshblack · 2020-02-28T21:27:22Z

packages/react/src/components/DataTable/TableToolbar.js

+  /**
+   * `normal` Change the row height of table
+   */
+  size: PropTypes.oneOf(['compact', 'short', 'normal', 'tall']),


Should we include all of these options if they aren't currently being used? 👀

Actually all are used. <TableToolbar> code translates compact/short table size to the small variant of tool bar, and normal/tall table size to the normal variant of tool bar.

It seems like only compact and short are being used, correct? The other values would noop?

In general, it seems like from the issue what we want to express is "display the small variant of toolbar if the table size is either compact or short"? Would it make sense for the size to then be either "small" or "normal" since that would refer to the height of the toolbar versus the size referring to the table? This logic could then come from the DataTable for determining the size in getToolbarProps if we want to use that

Moved the size translation logic to getToolbarProps() as it seems to have been suggested: fe23222

asudoh · 2020-03-02T08:20:10Z

@tw15egan @laurenmrice You are right that I forgot to check in the change in story. My apologies. Fixed.

@emyarod My apologies for the snapshot thing, too - Updated.

tw15egan · 2020-03-02T08:46:17Z

@asudoh should we also update the stories for with toolbar and possibly with dynamic content?

asudoh · 2020-03-02T08:56:06Z

@tw15egan Great catch - My apologies again for missing that. Fixed.

tw15egan

LGTM 👍 ✅

aagonzales

Reviewing for Lauren who is OOO today.
Looks like it's working correctly now 👍

emyarod

what should we do when the button content overflows? by default the button only has a min-height but since we are setting an explicit height on the compact toolbar and the toolbar button, we end up with this

asudoh · 2020-03-06T04:47:40Z

@emyarod I think application can set an alternate width via style override in such scenario. CC @carbon-design-system/design just in case we should enlarge the button width by default.

Technically we can add ellipsis, but I don't think doing so provides a good UX.

laurenmrice · 2020-03-06T15:19:30Z

We should allow the button to be able to change width based on the content inside of it. Like this for example:

asudoh · 2020-03-09T01:05:18Z

Thanks @laurenmrice for the guidance! Fixed.

laurenmrice · 2020-03-09T20:40:01Z

The with toolbar story at the compact and short sizes have a primary button with text misaligned. It seem fine in the other stories though.

I also have another question. Is this pr affecting just the regular toolbar or also the batch action toolbar? Because the batch action toolbar at short and compact have misaligned text.

asudoh · 2020-03-10T08:04:15Z

Good catch @laurenmrice! (Were FF-only issues) Fixed both.

laurenmrice

looks good to me ! thank you 🙌🏻

emyarod

looks good to me

feat(DataTable): add small toolbar

fa9cae6

This change adds small variant of table toolbar, used with compact/short variant of table rows. There were styles implemented for that presumably as part of original `v10` work, but it was not used somehow. Fixes carbon-design-system#5420.

asudoh added status: ready for review 👀 status: visual review 🔍 labels Feb 28, 2020

asudoh requested review from emyarod, tw15egan and a team February 28, 2020 08:36

asudoh requested a review from a team as a code owner February 28, 2020 08:36

asudoh requested review from laurenmrice and removed request for a team February 28, 2020 08:36

Merge branch 'master' into small-table-toolbar

94e749d

tw15egan reviewed Feb 28, 2020

View reviewed changes

tw15egan requested changes Feb 28, 2020

View reviewed changes

emyarod reviewed Feb 28, 2020

View reviewed changes

joshblack suggested changes Feb 28, 2020

View reviewed changes

asudoh added 3 commits March 2, 2020 14:36

chore(devenv): update DataTable story

dd7dd22

Merge branch 'master' into small-table-toolbar

d6ed270

test(DataTable): update snapshot

10b95a6

chore(devenv): more update DataTable story

72707bd

tw15egan approved these changes Mar 2, 2020

View reviewed changes

aagonzales approved these changes Mar 2, 2020

View reviewed changes

tw15egan and others added 2 commits March 3, 2020 15:58

Merge branch 'master' into small-table-toolbar

9217f11

chore(DataTable): move size translation logic to DataTable

fe23222

emyarod reviewed Mar 5, 2020

View reviewed changes

Merge branch 'master' into small-table-toolbar

8a9ae4b

asudoh added 2 commits March 6, 2020 13:29

Merge branch 'master' into small-table-toolbar

926a419

test(DataTable): update snapshot

18759ed

fix(data-table): auto width batch action button

3000dac

fix(data-table): fix button padding for Firefox

4fb3d3c

laurenmrice approved these changes Mar 10, 2020

View reviewed changes

Merge branch 'master' into small-table-toolbar

a9c7ec9

tw15egan added status: one more review 👀 and removed status: visual review 🔍 labels Mar 10, 2020

emyarod approved these changes Mar 10, 2020

View reviewed changes

joshblack approved these changes Mar 11, 2020

View reviewed changes

Merge branch 'master' into small-table-toolbar

1b99c97

asudoh merged commit ef9c347 into carbon-design-system:master Mar 11, 2020

asudoh deleted the small-table-toolbar branch March 11, 2020 01:38

joshblack mentioned this pull request Mar 31, 2020

chore(release): v10.11.0-rc.0 #5762

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(DataTable): add small toolbar #5479

feat(DataTable): add small toolbar #5479

asudoh commented Feb 28, 2020

netlify bot commented Feb 28, 2020 •

edited

Loading

netlify bot commented Feb 28, 2020 •

edited

Loading

tw15egan Feb 28, 2020

tw15egan left a comment

laurenmrice commented Feb 28, 2020

emyarod left a comment

joshblack Feb 28, 2020

asudoh Mar 2, 2020

joshblack Mar 4, 2020

joshblack Feb 28, 2020

asudoh Mar 2, 2020

joshblack Mar 4, 2020

joshblack Mar 4, 2020

asudoh Mar 5, 2020

asudoh commented Mar 2, 2020

tw15egan commented Mar 2, 2020

asudoh commented Mar 2, 2020

tw15egan left a comment

aagonzales left a comment

emyarod left a comment

asudoh commented Mar 6, 2020

laurenmrice commented Mar 6, 2020

asudoh commented Mar 9, 2020

laurenmrice commented Mar 9, 2020 •

edited

Loading

asudoh commented Mar 10, 2020

laurenmrice left a comment

emyarod left a comment

feat(DataTable): add small toolbar #5479

feat(DataTable): add small toolbar #5479

Conversation

asudoh commented Feb 28, 2020

Changelog

Testing / Reviewing

netlify bot commented Feb 28, 2020 • edited Loading

netlify bot commented Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

tw15egan left a comment

Choose a reason for hiding this comment

laurenmrice commented Feb 28, 2020

emyarod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asudoh commented Mar 2, 2020

tw15egan commented Mar 2, 2020

asudoh commented Mar 2, 2020

tw15egan left a comment

Choose a reason for hiding this comment

aagonzales left a comment

Choose a reason for hiding this comment

emyarod left a comment

Choose a reason for hiding this comment

asudoh commented Mar 6, 2020

laurenmrice commented Mar 6, 2020

asudoh commented Mar 9, 2020

laurenmrice commented Mar 9, 2020 • edited Loading

asudoh commented Mar 10, 2020

laurenmrice left a comment

Choose a reason for hiding this comment

emyarod left a comment

Choose a reason for hiding this comment

netlify bot commented Feb 28, 2020 •

edited

Loading

netlify bot commented Feb 28, 2020 •

edited

Loading

laurenmrice commented Mar 9, 2020 •

edited

Loading