Table actions visibility #1103

cchaos · 2018-08-08T21:05:44Z

Piggy-backing off of the discussion from #1080, here is a POC for altering the EuiBasicTable to utilize the concept agreed upon.

I've commented out all but the Actions example in the docs to help zero in on the relevant example. I tried not to create any breaking changes.

Essentially, instead of completely hiding the icons until hover, their opacity is just reduced a bit and the actions object now doesn't collapse until there are more than 2 (not more than 1) options. If any are deemed isPrimary, those will be shown on hover alongside the "more" dropdown, though only the first 2 "primary" actions will be shown in case more were declared.

The isPrimary prop added to the actions options, is my favorite prop name because it conflicts with our color scheme naming, but I'm having trouble coming up with a better one.

Decisions:

Icon color: Do we allow for supplied colors, restrict to a smaller palette, or not allow custom colors at all and they're always black or blue?

cjcenizal · 2018-08-08T22:24:46Z

Thanks so much for doing this @cchaos! I think this does a great job of addressing my original problem of not knowing if a table's rows can be edited or otherwise interacted with just by looking at the table.

I have a few thoughts, just to share my stream of consciousness:

We'll need to add tooltips for the icons (I think @snide mentioned that in the original issue).
I'm not sure if it makes sense to hide the actions when a row is selected. I'm leaning towards allowing the consumer to remove actions which conflict with having a selection instead of hardcoding that behavior.
isPrimary sounds pretty good to me, I can't think of a better name either -- this is a first! 😄
Does the contrast of the grayed-out version of the icons pass accessibility requirements? I think it's OK if disabled items like the pagination arrows don't pass since they can't be interacted with, but it seems important that people are able to distinguish this visual signifier. Maybe we can darken the gray / increase the opacity to match the un-hovered gray of the toast's close button?

cchaos · 2018-08-09T14:03:28Z

Thanks @cjcenizal !

Yeah, we talked about tooltips as well, and I had mentioned that I was worried about the judicious use of tooltips all over the UI and especially in repeatable elements like table rows. I'd like to say we can just rely on the title attribute to give us the context we need without overloading the user with tooltips. Though, I think the pause before the title attribute shows may be a little too long, and I think Bill had mentioned that they tend to be too small. We then decided it might be best to allow tooltip delay to be customized and in repeatable elements like this, we can give it an extra beat before showing the tooltip, so they don't go crazy all over the page.
I'm actually not hiding the actions based on row selection but based on the disabled state of the button (which in this example, disables them when a selection is made). Though I think I should just not show any completely hidden disabled icons on hover and make sure the grayed out ones don't increase their opacity.
😆
Yeah I'll have to re-check for contrast before we finalize.

I've also started adding some decision points to the main description of this PR.

cchaos · 2018-08-09T19:16:07Z

Ok, so I just pushed a couple commits, that address items 1 & 2 above.

I added a delay prop to EuiToolTip and wrapped the action items with a tooltip if enabled and has description.

Before delay

After delay

Not if disabled

Only hiding completely hidden actions when disabled

The issue I'm having with item 2 is that, for the collapsed (all actions popover), the disabled prop gets passed down 3 levels to the trigger button, so the :disabled attribute isn't on the same element as the .euiTableCellContent__hoverItem....

ryankeairns · 2018-09-04T21:16:19Z

Everything works as described above. LGTM!

cc/ @snide

and wrapped action items with tooltip **if enabled and has description**

snide · 2018-09-06T20:58:31Z

I tested this pretty thoroughly in Kibana master using a yarn link. These are the only places where I saw a different display. All other tables seem unaffected.

Saved objects

Likely happening because their are exactly two items.

Before

After

ML jobs list

This is because they are adding some extra bordering on things. Since the ML team requested this change, I think it's OK to expect them to convert their tables to use this stuff. The break below is purely visual and does not affect functionality in any way.

Before

After

Conclusion

We might want to label this one breaking. The changes look really easy to fix, but I don't have visibility into what the other teams are using this stuff for (thinking of cloud). Either that or we might want to rethink the 2 actions scenario.

cchaos · 2018-09-06T22:53:22Z

So it's mostly going to affect tables that have exactly 2 actions which are currently auto-populating into a popover so they didn't need to add the type: 'icon' to each action. So tables like the saved objects one will just need to add that.

That ML issue actually comes from a different issue, where EuiButtonIcon's now have a min-width and they were setting the widths of these to 18px (which is a no-no anyway).

I'll set it as a breaking change, but it's not really breaking functionality, so I'm not too worried about changing it in general.

- Changed ‘person’ to ‘user’ - Basic table now auto-applies `hasActions` and `isSelectable` to rows that provide those column types, but it’s still configurable too - Addressed opacity accessibility

cchaos · 2018-09-10T20:18:32Z

Ok, this is ready for final (and code) review.

chandlerprall

Two small changes, otherwise code and visuals look great

chandlerprall · 2018-09-11T18:05:26Z

src/components/basic_table/basic_table.js

@@ -590,9 +597,9 @@ export class EuiBasicTable extends Component {
      <Fragment key={`row_${itemId}`}>
        <EuiTableRow
          aria-owns={expandedRowId}
-          isSelectable={isSelectable}
+          isSelectable={typeof isSelectable === 'boolean' ? isSelectable : calculatedHasSelection}


{isSelectable == null ? calculatedHasSelection : isSelectable}
IMO is easier to read, and it more clearly communicates that the variable is null vs. a value, instead of multiple kinds of values.

Makes sense, done

chandlerprall · 2018-09-11T18:05:28Z

src/components/basic_table/basic_table.js

          isSelected={selected}
-          hasActions={hasActions}
+          hasActions={typeof hasActions === 'boolean' ? hasActions : calculatedHasActions}


same change here

chandlerprall

LGTM! Code review, pulled and tested locally, looks great!

snide · 2018-09-11T19:22:27Z

Let's talk about the "breaking" changes in this quickly before we merge, might want to do it as a separate release after we push a new one.

cchaos · 2018-09-12T18:33:47Z

Understanding the breaking change

Up to two visible actions per row (not including the "More")

Essentially, instead of completely hiding the icons until hover, their opacity is just reduced a bit and the actions object now doesn't collapse until there are more than 2 (not more than 1) options. If any are deemed isPrimary, those will be shown on hover alongside the "more" dropdown, though only the first 2 "primary" actions will be shown in case more were declared.

Remedies

If your table had exactly 2 actions and you are now seeing your table actions displayed as full buttons, add type: 'icon' as well as an icon type to each action.

If your table had more than 2 actions and you now see the first two action as full buttons next to an ellipses, add type: 'icon' as well as an icon type to at least 2 actions. You'll also want to supply those two actions with isPrimary which will ensure those actions are the ones displayed in the row.

cchaos mentioned this pull request Aug 8, 2018

[DISCUSS] Make EuiBasicTable actions always visible #1080

Closed

cchaos changed the title ~~[WIP] Table actions visibility~~ Table actions visibility Sep 6, 2018

cchaos mentioned this pull request Sep 6, 2018

[Spaces] - Handle space renaming and deleting elastic/kibana#22586

Merged

cchaos added 3 commits September 6, 2018 15:40

Initial concept

64f6d71

Only hiding completely hidden actions when disabled

c0afae7

Added a delay option to tooltip

c179b97

and wrapped action items with tooltip **if enabled and has description**

cchaos force-pushed the poc/table-actions branch from 26c584e to c179b97 Compare September 6, 2018 19:41

updating snaps

8b2919d

cchaos added 4 commits September 7, 2018 15:11

Addressing PR comments

0509d9c

- Changed ‘person’ to ‘user’ - Basic table now auto-applies `hasActions` and `isSelectable` to rows that provide those column types, but it’s still configurable too - Addressed opacity accessibility

Fixing up docs and empty button as default

3f1e48b

Merge remote-tracking branch '2_upstream/master' into poc/table-actions

e7f52c2

changelog

98ff837

cchaos requested review from snide and chandlerprall September 10, 2018 20:18

Merge branch 'master' into poc/table-actions

5df355c

chandlerprall suggested changes Sep 11, 2018

View reviewed changes

More readable logic

9955932

chandlerprall approved these changes Sep 11, 2018

View reviewed changes

Merge branch 'master' into poc/table-actions

2ee65a7

cchaos merged commit 08f2d45 into elastic:master Sep 12, 2018

cchaos deleted the poc/table-actions branch September 12, 2018 18:09

snide added the feature request label Sep 12, 2018

snide added the breaking change label Sep 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Table actions visibility #1103

Table actions visibility #1103

cchaos commented Aug 8, 2018 •

edited

Loading

cjcenizal commented Aug 8, 2018

cchaos commented Aug 9, 2018 •

edited

Loading

cchaos commented Aug 9, 2018

ryankeairns commented Sep 4, 2018

snide commented Sep 6, 2018

cchaos commented Sep 6, 2018

cchaos commented Sep 10, 2018

chandlerprall left a comment

chandlerprall Sep 11, 2018

cchaos Sep 11, 2018

chandlerprall Sep 11, 2018

chandlerprall left a comment

snide commented Sep 11, 2018

cchaos commented Sep 12, 2018

Table actions visibility #1103

Table actions visibility #1103

Conversation

cchaos commented Aug 8, 2018 • edited Loading

Decisions:

cjcenizal commented Aug 8, 2018

cchaos commented Aug 9, 2018 • edited Loading

cchaos commented Aug 9, 2018

ryankeairns commented Sep 4, 2018

snide commented Sep 6, 2018

Saved objects

Before

After

ML jobs list

Before

After

Conclusion

cchaos commented Sep 6, 2018

cchaos commented Sep 10, 2018

chandlerprall left a comment

Choose a reason for hiding this comment

chandlerprall Sep 11, 2018

Choose a reason for hiding this comment

cchaos Sep 11, 2018

Choose a reason for hiding this comment

chandlerprall Sep 11, 2018

Choose a reason for hiding this comment

chandlerprall left a comment

Choose a reason for hiding this comment

snide commented Sep 11, 2018

cchaos commented Sep 12, 2018

Understanding the breaking change

cchaos commented Aug 8, 2018 •

edited

Loading

cchaos commented Aug 9, 2018 •

edited

Loading