Responsive tables #584

cchaos · 2018-03-28T00:37:03Z

This one's a doozy.

Basic initialization of responsive table
- Uses header prop to add data-header to table cell and ouput content on small screens.
Allowing for custom row headers
- hideforMobile: doesn’t display the cell at xSmall breakpoint
- isMobileHeader: doesn’t display data-header attribute but enlarges the text and displays it full width
Adding support for selections in mobile
- add isSelectable to a row and it will absolutely position the check box on the far left
Keeping pagination on one line (no responsive)
Created mobile context menu for filtering
- EuiTableSortMobile for the whole context menu
- EuiTableSortMobileItem for individual buttons
- BUG: The items don’t update their render on click fixed
- TODO: Add text to the trigger button signifying which column is being sorted on (will file separate issue)
Scrolling to top of table when:
- page changed
- num of pages changed
- BUG: scrolls up then back down because the button keeps focus (will file separate issue)
Added support for actions in mobile
- Must add hasActions to the BasicTable or Row to properly position the action buttons.
- Q: On mobile should the action buttons always be visible? A: Yes
Modifying the Custom table example
- Added the component EuiTableHeaderMobile to encapsulate multiple small screen only table options (sort/select all, etc…)
- Moved the other Mobile comps into the subfolder /mobile
Allowing cells in mobile view to be explicitly 100% wide
- Added isMobileFullWidth as a prop to EuiTableRowCell
Added support for expanding rows
- Added some basic styling to expandable rows Provide some default design for expanding table rows #587, could use some more work on the transition and font size inheritance side.
- Must add the prop isExpandedRow to the EuiTableRow to get the right class.

To Do

Test across browsers
Find all instances of tables and update (in documentation)
For some reason the Custom table example is really lagging in mobile view

Feedback greatly appreciated.

I also welcome the opportunity to pair on this one.

snide · 2018-03-28T21:10:26Z

This is great. The layout you have in place I think works well.

Some random thoughts on the design bits.

Maybe bump the font back up when in mobile mode. It feels a little tight on the phone with the 14px. This also gives it better contrast against the new label.
Possibly mimic carding for better separation of elements on the rows themselves? I'm torn on this, since I don't like boxing stuff, but it feels like it helps group the content when im on my phone.
We should probably drop the background hover in mobile, since it's only in there on Desktop for scannability of long rows.
For selection, maybe we could fix position the action bar at the bottom of the page so you don't need to scroll all the way back up. Otherwise we could always remove / hiding selections as something you can do in mobile mode. The action items should always be there right?
I'd make sorting a full width element button similar to how we handle the side nav. Feels a little lost on the right side.
We have a lot of work to do with popovers (not tied to this PR obviously). Any ideas on how we could make them better in mobile other than positioning? Was thinking maybe we could make them up like we do with modals?

cchaos · 2018-03-28T21:41:31Z

Awesome, thank you for the feedback. In regards to your points:

bump the font back up: I agree and can do that.
mimic carding: I like that idea, mainly for the fact that I didn't like that the table borders didn't extend the full width of the container and I was having trouble spotting the new rows. This approach basically turns them into cards which is basically what they are in this layout.
drop the background hover: I'm not sure that's necessary since "hover" doesn't exist in mobile anyway. I do like that it still has the focus background color.
fix position the action bar: YES. And I thought about trying to go ahead and create that, but I think there's a bigger picture component here that's something like <EuiMobileBar> that basically behaves like a <EuiBottomBar> but only exists at mobile sized screens. I wasn't sure the best approach to this. @snide Maybe we could pair on creating something like this?
make sorting a full width button: The reason I didn't was because there can also be other header actions like "Select all"

popovers (not tied to this PR obviously): Yes I think we could make them like dismissable toasts that are attached to the bottom (same with context menus). Again this is something where a <EuiMobileBar> would come in handy to wrap the components in.

snide · 2018-03-28T22:15:42Z

Ooh. I like your halfsy toast idea for popovers.

jen-huang · 2018-04-05T09:24:23Z

I opened a PR to adjust some of the JS, details: cchaos#2
I will continue to take a closer look at the JS, but nothing major has jumped out at me :)

Noticed a couple of things while I was working on that:

The focus state of the mobile sorting popover could use some padding. Maybe remove flushRight and increase size? (Also would it be better to say Sort instead of Sorting?

Offline/Online status field in Adding selection to a BasicTable doc example is overlapping the selection area on right.

cchaos · 2018-04-16T21:40:48Z

Aaaand done! Ready for reviews...

snide

This is a biggun and a very hard problem to solve! Nice work.

I'll do a visual review first, since the code will take a bit.

Currently you're lowering the opacity of action buttons when they are disabled (after a selection of a different row). I think you might want to remove them (or set opacity: 0) instead. On a phone it's not going to be as apparent you can't click them because you won't have the lack of hover state context. You'll just see a gear and wonder why it can't be clicked. Though I could be overthinking it.

Should Basic and In-Memory table carry the same styling for checkbox selection? Right now one of them puts the checkboxes on the right, the other on the left. Think it looks good always on the right with the line that you have added.

For the expanding rows bit I'm wondering if the expanded row should replace the original row completely. Right now they are a little disjointed and look separate. If we keep the original around (and I haven't looked at the code, it likely requires it that way now), then we should figure out some way to visually tie the two rows together as a unit by adding some sort of wrapping border or shelving. Just something to say... hey, these two are together.

On your mobile version of the same component it works better because of the wrapper. Whatever you do with desktop, you can likely loose the gray shading for the expanded bit here.

Some weird spacing happening in the messaging for loading table items.

cjcenizal

This is really impressive! The code looks great overall. I had a few comments. I also have some feedback on the design, take it for what you will.

Sorting

Is it broken? I can't figure out how to get it to work on the docs site:

Pagination

When I click the arrows, I'm using to seeing the pagination itself reflect the state change somehow. Maybe if it said "1/7" (current page / total pages) that would help, because then the number would change as I change the page.

Selection

This is a great solution, I just have a hard time with the placement of the checkbox because it's so disassociated with the content of the row itself. Now I have to scan the checkboxes on the right (I'm used to scanning content on the left), and then I need to zig my eyes left to scan and see which one is selected. I know we have the background color as a selection signifier, but the checkbox is a strong visual anchor and really aids scanning. If we put it on the left, we could put the actions on the top right. If the row is expandable, the expansion arrow could go below the checkbox, or even at the bottom left?

cjcenizal · 2018-04-17T00:44:30Z

src-docs/src/views/tables/expanding_rows/expanding_rows_section.js

@@ -25,6 +25,8 @@ export const section = {
    <p>
      You can expand rows by passing in a <EuiCode>itemIdToExpandedRowMap</EuiCode> prop
      which will contain the content you want rendered inside the expanded row.
+      You will also need to add the prop <EuiCode>isExpandedRow</EuiCode> to the row that


Maybe add a note that this only applies if you're building out a custom table, but not if you're using basic table or in-memory table?

cjcenizal · 2018-04-17T00:45:52Z

src/components/basic_table/basic_table.js

@@ -167,6 +169,11 @@ export class EuiBasicTable extends Component {
    return criteria;
  }

+  scrollToTop = () => {
+    const topOfTable = this.tableElement.offsetTop;
+    window.scrollTo(0, topOfTable);


~~To be really future-proof, can we do window.scrollTo(window.scrollX, topOfTable) since we don't want to affect the horizontal scroll at all?~~

After playing with this a little, I think we should remove this functionality. I totally understand the intention, especially on mobile... but the experience on the docs site is a little jarring when on a desktop, especially if I want to keep paging. Have you already experimented with tables in either Kibana or Cloud to identify pain points in our actual use cases? If not then it might make sense to remove it for now... it will be easy to add it back in if/when we discover that we really need it.

That's fine. It's not a smooth scroll like I would like it to be anyway. But we should definitely keep it in mind once we see more users using mobile.

cjcenizal · 2018-04-17T00:55:30Z

src/components/context_menu/context_menu_panel.js

@@ -219,6 +220,22 @@ export class EuiContextMenuPanel extends Component {
    }
  }

+  getWatchedProps(items) {


Could you add some comments explaining what this is for?

@jen-huang Should be able to help with that.

@cjcenizal Added some inline comments to getWatchedProps() and shouldComponentUpdate().

Previously, EuiContextMenuPanel doesn't re-render if its items changes, due to custom logic in shouldComponentUpdate(). For mobile tables, there is a dropdown of sortable fields, using this component, that displays field name (unchanging) and sort direction (changing!). After changing sorting, the UI wouldn't update until dropdown is closed and re-opened.

To fix this I added the prop watchedItemProps, which specifies what properties of items the component should check for changes as part of shouldComponentUpdate(). In this case we are watching isSorted and isSortAscending.

Thanks Jen! That makes a lot of sense. Very nice solution.

Could I make a small suggestion? I think changing the name slightly, adding a quick comment re intent, and and doing the stringification internally would make this a bit easier to grok. What do you think?

getWatchedPropsForItems(items) { // This lets us compare prevProps and nextProps among items so we can re-render if our items const { watchedItemProps } = this.props; // Create fingerprint of each item's watched properties if(items && items.length && watchedItemProps && watchedItemProps.length) { return JSON.stringify(items.map(item => { // Create object of item properties and values const props = { key: item.key, }; watchedItemProps.forEach(prop => props[prop] = item.props[prop]); return props; })); } return null; }

@cjcenizal Updated!

Nice, thanks!

cjcenizal · 2018-04-17T00:56:57Z

src/components/table/mobile/_index.scss

+      display: none; // Use mobile versions of selecting and filtering instead
+    }
+
+    // Not allowing compressed styles in mobile view (for now)


How about deleting the commented-out styles, and moving this comment up to the .euiTable--responsive class?

cjcenizal · 2018-04-17T00:57:32Z

src/components/table/mobile/table_sort_mobile_item.js

+  });
+
+    const columnTitle = ariaLabel ? ariaLabel : children;
+    const statefulAriaLabel = `Sort ${columnTitle} ${isSortAscending ? 'descending' : 'ascending'}`;


Looks like these variables are over-indented a bit.

cchaos · 2018-04-17T05:28:23Z

@snide and @cjcenizal thank you for the feedback here are a few responses:

Currently you're lowering the opacity of action buttons when they are disabled (after a selection of a different row).

This seems like it was actually a carry-over from the non-mobile version, but I can certainly just hide them completely.

Should Basic and In-Memory table carry the same styling for checkbox selection?

Yes, I just forgot to add the isSelectable prop to that table

For the expanding rows bit I'm wondering if the expanded row should replace the original row completely.

Yeah, they're 2 separate tr's and I was having trouble coming up with a good styling solution. Let's push the more intricate styling off to another ticket.

Some weird spacing happening in the messaging for loading table items.

Yeah that weird spacing has to do with the td adding the header content in a :before but the data-attr doesn't exist. I need some help coming up with a solution for this case and using :empty didn't work.

Sorting. Is it broken?

Yes/No, when I added the responsive stuff as optional I forgot to also set the EuiBasicTable responsive prop to default to true.

Pagination numbering

I agree and I already made a separate ticket: #606

Selection. This is a great solution, I just have a hard time with the placement of the checkbox because it's so disassociated with the content of the row itself.

Yeah I've been bouncing back and forth and sideways about where to place these and looking one with all 3 options, I'm thinking that the checkbox should go on the left, actions in the top right, and expander in the bottom right. I'll make that change.

snide

Code here looks pretty readable. Left some small notes, but it's up to you whether you wanna make the changes.

I would recommend rewriting your example docs intro. The information is good, but I'd format it a little different to say the equiv of...

To make your table work responsively, please make sure you utilize the following additional props:

Right now it's a little unclear that these are additive things you need to add to your table to make them work (at their best) in mobile. Basically that's you're designing a better view.

Looks like this branch may have inadvertently broke the props documentation. I'd love to see how that stuff before this gets in just to see if all the new prop API is clear when browsing it from that view.

snide · 2018-04-17T21:41:58Z

src/components/table/_table.scss

+  animation: $euiAnimSpeedNormal $euiAnimSlightResistance 1 normal forwards growExpandedRow;
+}
+
+@keyframes growExpandedRow {


We'll need to figure out something better eventually. I did a similar trick with the accordion / sidebar, but it definitely isn't very performant. With all the table stuff it clunks up a bit.

I don't have any better ideas. Think it would have to be a transform scaling trick.

Yeah I'm pushing that all off to a different ticket.

snide · 2018-04-17T21:48:20Z

src/components/table/mobile/_index.scss

@@ -0,0 +1,195 @@
+@import '../../panel/index';


We might want to separate out the mixins we need in panel similar to the way we're doing forms. This might dupe the entire panel css. That way you could just include panel/mixin.

Yeah it's already separated, I think I just imported index to quickly check usage and forgot to change it to the right file.

snide · 2018-04-17T21:57:42Z

src/components/table/mobile/_index.scss

+    // Make each row a Panel
+    @include euiPanel($selector: 'euiTableRow');
+
+    .euiTableRow {


Should this stuff be in table.scss with it's desktop equiv? The folder for /mobile/ mostly deals with new mobile specific components, but the Sass file does a bunch of overwriting of stuff down at the base layer. Usually we've included mobile overwrites in the same files to make it a little easier to read the overwrite.

I'm not tied to it, more just a general question. How do you think we should do this? If we leave it here, I think I'd at least wanna see some comments in the other files telling me where to go.

Also, we've generally used the _index.scss file as a pure import root. Again, don't think it's a big deal, just pointing stuff out and seeing what you think we should do.

The reason I moved it was because I hate 300 line SASS files. Makes them feel less readable to me. Though, I can at least put it in a _responsive.scss file that's in the table root folder and leave the mobile file for strictly those new components.

You're right about the index file, I'll change that.

snide · 2018-04-17T22:00:28Z

src/components/table/table_header_cell.js

@@ -98,6 +103,8 @@ EuiTableHeaderCell.propTypes = {
  isSorted: PropTypes.bool,
  isSortAscending: PropTypes.bool,
  scope: PropTypes.oneOf(['col', 'row', 'colgroup', 'rowgroup']),
+  isMobileHeader: PropTypes.bool,


Can you add prop docs here. I know you have some stuff in the example, but just nice for the autodocs as well.

snide · 2018-04-17T22:00:38Z

src/components/table/table_header_cell.js

@@ -98,6 +103,8 @@ EuiTableHeaderCell.propTypes = {
  isSorted: PropTypes.bool,
  isSortAscending: PropTypes.bool,
  scope: PropTypes.oneOf(['col', 'row', 'colgroup', 'rowgroup']),
+  isMobileHeader: PropTypes.bool,
+  hideForMobile: PropTypes.bool,


snide · 2018-04-17T22:01:29Z

src/components/table/table_row.js

@@ -20,5 +33,9 @@ export const EuiTableRow = ({ children, className, isSelected, ...rest }) => {
 EuiTableRow.propTypes = {
  children: PropTypes.node,
  className: PropTypes.string,
+  isSelectable: PropTypes.bool,


Same on all the row props too if you have time.

snide · 2018-04-17T22:02:01Z

src/components/table/table_row_cell.js

-  colSpan: PropTypes.number
+  colSpan: PropTypes.number,
+  header: PropTypes.string,
+  hideForMobile: PropTypes.bool,


And this set as well.

snide · 2018-04-17T22:02:31Z

src/components/table/mobile/table_sort_mobile_item.js

+EuiTableSortMobileItem.propTypes = {
+  children: PropTypes.node,
+  className: PropTypes.string,
+  onSort: PropTypes.func,


Auto prop docs.

cchaos · 2018-04-18T16:51:09Z

@snide Real quick reply, this branch did not break the width of the props tables. This is a screenshot of the published docs:

snide · 2018-04-18T17:24:41Z

@cchaos Sorry I wasn't more clear. Notice how the live one has bolded items for the props name, and the column seems to be missing in your branch.

master

branch

snide · 2018-04-18T17:27:38Z

Looks like something with the mobile header.

cchaos · 2018-04-18T17:35:11Z

Oh yeah good call, I guess I wasn't thinking about the application of using the mobile header just for styling and forgot that it would hide that column in non-mobile. I'll fix.

cchaos · 2018-04-18T19:13:00Z

@snide Ok, I think I addressed all your comments.

- Uses `header` prop to add `data-header` to table cell and ouput content on small screens.

- `hideforMobile`: doesn’t display the cell at xSmall breakpoint - `isMobileHeader`: doesn’t display `data-header` attribute but enlarges the text and displays it full width

- add `isSelectable` to a row and it will absolutely position the check box on the far left

…er can be closed. Fix centering of loading error message.

…r ContextMenuPanel when sorted

- Shifted checkbox, expander, and actions around - Hiding actions if a row is selected - Some code cleanup

I can’t remember why I added it (might have been for debugging). I removed it and it didn’t seem to have any ill-consequences.

This reverts commit 03ce998802e6ef73386fc13a592d85e76ec63f4c.

I remembered why, it is to align cell contents in case a cell has no header.

- Fixing missing column in non-mobile of docs props table - Altering docs intro - Adding more props comments - Adding more props tables to responsive and custom (BUG: some components don’t seem to populate a props table)

- Importing panel mixins not index file - Putting table responsive stuff in a _responsive file - Only mobile component stuff in the _mobile.scss imported from it’s index

cchaos · 2018-04-18T20:25:36Z

jenkins test this

snide

Tested this out with @cchaos in Kibana by using a yarn link. If you don't use the additional props provided in this PR, the tables can get a little ugly (though no uglier than they are now in mobile). WIth the addition of pseduo-titles it also means people will need to manually provide that content to the custom components if they go the custom table route (as is the case in the index management table in Kibana).

I'm gonna make an issue asking for some engineering help to make some of these things auto-apply so implementors don't need to worry about it, but I think it can wait for a follow up PR. Stuff like hasActions and isSelectable feels like stuff that can be automatically added based on the content. Till then, adding two lines to your tables ain't no big deal.

I do think we should probably list it as a Breaking change just so people are aware of it when scanning the changelog. It doesn't technically break, but listing it as such will make people check this stuff out.

@cjcenizal and @bevacqua should do a similar scan over in cloud when they start using this.

For the immediate we'll just need to make people aware they need to make sure to add these props to their tables to make them awesome in mobile.

Party time? Nice work @cchaos, this was a very hard problem.

cchaos added the work in progress label Mar 28, 2018

cchaos force-pushed the responsive-tables branch from cf3ceb5 to 5d0bdd0 Compare March 28, 2018 22:04

jen-huang self-requested a review April 5, 2018 09:03

cchaos force-pushed the responsive-tables branch from b4264aa to a68d0e9 Compare April 16, 2018 18:48

snide mentioned this pull request Apr 16, 2018

Provide some default design for expanding table rows #587

Closed

cchaos requested review from snide and cjcenizal April 16, 2018 21:41

cchaos added feature request assign:anyone and removed work in progress labels Apr 16, 2018

snide removed the assign:anyone label Apr 16, 2018

snide reviewed Apr 16, 2018

View reviewed changes

cjcenizal reviewed Apr 17, 2018

View reviewed changes

jen-huang removed their request for review April 17, 2018 18:16

snide suggested changes Apr 17, 2018

View reviewed changes

snide mentioned this pull request Apr 18, 2018

Add compressed props on basic/memory tables #687

Merged

cchaos added 3 commits April 18, 2018 16:20

Basic initialization of responsive table

ed6671b

- Uses `header` prop to add `data-header` to table cell and ouput content on small screens.

Allowing for custom row headers

55a54fa

- `hideforMobile`: doesn’t display the cell at xSmall breakpoint - `isMobileHeader`: doesn’t display `data-header` attribute but enlarges the text and displays it full width

Adding support for selections in mobile

b514a61

- add `isSelectable` to a row and it will absolutely position the check box on the far left

cchaos and others added 22 commits April 18, 2018 16:20

Compressed !== true

d3ef1df

Make EuiTableSortMobile render EuiTableSortMobileItems, so that popov…

c6807ba

…er can be closed. Fix centering of loading error message.

Fix TableSortMobile re-rendering, remove hideForMobile flag, re-rende…

95f907a

…r ContextMenuPanel when sorted

Fixing padding side and rebase issue

ed4ea6e

Positioning actions always on the right

37b02ea

Using new class selector to always display hover items

934dd8e

Putting loading indicator on tbody so it’s visible on mobile too

b04fa4b

Allowing tables to not be responsive

a78f65a

Updating docs

9fabb04

updating tables in button guidelines doc

b28de1b

Setting BasicTable default prop responsive = true

789812f

Added responsive props to EuiInMemoryTable

5370bad

Addressing PR feedback

5d2b324

- Shifted checkbox, expander, and actions around - Hiding actions if a row is selected - Some code cleanup

Added extra comment in docs

d8f3e15

Add comments

ba46ee1

Removing min-height on psuedo title

0729da1

I can’t remember why I added it (might have been for debugging). I removed it and it didn’t seem to have any ill-consequences.

Revert "Removing min-height on psuedo title"

1e33894

This reverts commit 03ce998802e6ef73386fc13a592d85e76ec63f4c.

Removing min-height of cell header if it’s the only cell that exists

ab4380d

I remembered why, it is to align cell contents in case a cell has no header.

Fixing exclusivity of isExpandable and hasActions

16d0db5

Change function name, move stringify to inside getWatchedPropsForItems

5ac259f

Addressing more PR requests

e83cd52

- Fixing missing column in non-mobile of docs props table - Altering docs intro - Adding more props comments - Adding more props tables to responsive and custom (BUG: some components don’t seem to populate a props table)

Adressing SASS PR feedback

7330bb0

- Importing panel mixins not index file - Putting table responsive stuff in a _responsive file - Only mobile component stuff in the _mobile.scss imported from it’s index

cchaos force-pushed the responsive-tables branch from 3b643d2 to 7330bb0 Compare April 18, 2018 20:20

changelog

64ab9ec

snide approved these changes Apr 18, 2018

View reviewed changes

cchaos merged commit d33c2ce into elastic:master Apr 18, 2018

cchaos deleted the responsive-tables branch April 18, 2018 20:35

snide mentioned this pull request Apr 19, 2018

EuiTable mobile properties should be automatically defined #691

Closed

jen-huang mentioned this pull request Apr 20, 2018

ContextMenuPanel not updating when it should #674

Closed

Responsive tables #584

Responsive tables #584

Conversation

cchaos commented Mar 28, 2018 • edited Loading

To Do

Feedback greatly appreciated.

I also welcome the opportunity to pair on this one.

snide commented Mar 28, 2018

cchaos commented Mar 28, 2018 • edited Loading

snide commented Mar 28, 2018

jen-huang commented Apr 5, 2018

cchaos commented Apr 16, 2018

snide left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

Sorting

Pagination

Selection

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

cjcenizal Apr 17, 2018 • edited Loading

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

cchaos commented Apr 17, 2018

snide 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchaos commented Apr 18, 2018 • edited Loading

snide commented Apr 18, 2018

master

branch

snide commented Apr 18, 2018

cchaos commented Apr 18, 2018

cchaos commented Apr 18, 2018

cchaos commented Apr 18, 2018

snide left a comment

Choose a reason for hiding this comment

cchaos commented Mar 28, 2018 •

edited

Loading

cchaos commented Mar 28, 2018 •

edited

Loading

cjcenizal Apr 17, 2018 •

edited

Loading

cchaos commented Apr 18, 2018 •

edited

Loading