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

Multiple fixes for mobile tables #1462

Merged
merged 15 commits into from
Feb 25, 2019
Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jan 22, 2019

1. Better singular prop object

mobileOptions object accepts the following:

prop type default note
show bool true If false, will not render the cell at all for mobile
only bool   Only show for mobile? If true, will not render the column at all for desktop
render node   Custom render/children if different from desktop
header node or bool   The column's header for use in mobile view (automatically passed down when using EuiBasicTable). Or pass false to not show a header at all.
enlarge bool   Increase text size compared to rest of cells
fullWidth bool   Allocates 100% of the width of the container in mobile view (typically cells are contained to 50%)

That means the following props are being deprecated:

prop use instead
header  mobileOptions.header
isMobileHeader mobileOptions.only = true & mobileOptions.header = false
hideForMobile mobileOptions.show = false
isMobileFullWidth mobileOptions.fullWidth

Example

{
  field: 'firstName',
  name: 'First Name',
  truncateText: true,
  mobileOptions: {
    render: (item) => (<span>{item.firstName} {item.lastName}</span>), // Custom renderer for mobile view only
    header: false,   // Won't show inline header in mobile view
    fullWidth: true, // Forces 100% width of the cell
    enlarge: true,   // Increase text size compared to rest of cells
    truncateText: false, // You can also pass any standard cell props but they only work if `render` is also provided
  }
}

2. The mobileOptions.header prop accepts a node for rendering

... Instead of using the CSS content: attr(data-header) trick. It's displayed directly in the DOM. Fixes #1315

screen shot 2019-01-22 at 17 51 02 pm

cc @cjcenizal

3. EuiBasicTable properly adds support for the mobile version of "Select all"

Fixes #1036


Deprecation Notice

Again, this deprecates some of the responsive-only props in favor of the mobileOptions object. However, it is currently not a breaking change. The old props will still work, but the object will override them if both are provided.

Testing

So I've left all but the Responsive and Custom examples as they were, to ensure that these are still working. Then when I get approval of the new object prop, I will update all the examples to use the proper prop.

I also ran this through Kibana via yarn link and went through a bunch of table-heavy pages and didn't come up with any breaks. Though I didn't run it through any tests.


Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • [ ] This required updates to Framer X components

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Should we trigger a console.warn if a deprecated prop is used?

src/components/table/table_row_cell.js Show resolved Hide resolved
src/components/basic_table/basic_table.js Show resolved Hide resolved
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

small TS change, otherwise proptypes and ts defs look great!

src/components/table/index.d.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Looks great!

@cchaos
Copy link
Contributor Author

cchaos commented Jan 23, 2019

Sweet. Ok, then I'll update all other table examples to use the proper props.

@cchaos cchaos force-pushed the responsive-tables branch 2 times, most recently from 235f90a to aa54cc8 Compare February 21, 2019 23:15
@cchaos cchaos added breaking change deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. labels Feb 21, 2019
@cchaos
Copy link
Contributor Author

cchaos commented Feb 21, 2019

Ok, I think I'm done with this PR finally. 🥇

@chandlerprall Can you do a double check mainly just on this file: https://github.com/elastic/eui/pull/1462/files#diff-cca6a37365f48c17b61c1ad941a710c1 and especially concerning the select all checkbox where I ended up having to append a makeId() because there were issues with multiple tables on the same page with the same checkbox id and label for.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes still LGTM

@cchaos cchaos merged commit 898d5e6 into elastic:master Feb 25, 2019
@cchaos cchaos deleted the responsive-tables branch February 25, 2019 20:56
@snide snide mentioned this pull request Feb 28, 2019
33 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change deprecations Contains deprecations. Add them to the deprecations meta ticket after merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants