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

[EuiDataGrid] Custom column headers #2421

Conversation

chandlerprall
Copy link
Contributor

Summary

Adds an optional display field onto the datagrid's column type which allows for any custom ReactNode

custom column headers

@chandlerprall chandlerprall changed the base branch from master to feature/euidatagrid October 11, 2019 15:45
@myasonik
Copy link
Contributor

With custom headers we finally have to resolve the long standing TODO of allowing users to focus on header cells as well.

If any header cell has a tabbable child, every header cell should allow users to focus on it in the same way the rest of the cells work. Also, not sure what will "just happen" when that's implemented but, the initial cell that a user enters the grid at should stay/become the first focusable cell (be it a header cell or body cell).

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM, works as expected.

As long as we have that TODO tracked somewhere for implementation, we're good to merge

@chandlerprall
Copy link
Contributor Author

I'll take a shot at navigating to the header row in this PR

@chandlerprall
Copy link
Contributor Author

Added header navigation, I think I got all the edge cases for it.

interactive header

  • when any header cell contains interactive elements, enable navigation to the header cells
  • if a header cell containing one interactive element receives focus, the focus shifts to its interactive element
  • if the header cell contains more than one interactive element, focus stays on the cell until enter or f2 are pressed
  • if a header cell's interactive element has focus, f2 and escape moves focus to the cell
  • navigating out of a cell via arrow keys disables that cell's elements' tabIndex
  • focusing off of a cell via tab or by clicking elsewhere in the document disables that cell's elements' tabIndex

I'm assuming I've missed something, so I'll add tests after approvals.

@chandlerprall
Copy link
Contributor Author

@snide the header's focus state needs a design pass; I stole the styles straight from the body cells.

@snide
Copy link
Contributor

snide commented Oct 14, 2019

OK. I'll give you a review / PR todayish.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Only odd thing I noticed was when multiple interactive elements are present:

  • arrow to 'name' column header cell
  • enter focuses 'email' icon button
  • tab focuses 'menu' icon button
  • tab jumps focus out of table to row pagination button

Not exactly sure what it should do. Cycle back to the 'email' icon button? Place focus back on the cell?

@chandlerprall
Copy link
Contributor Author

@myasonik any thoughts on focus trap or similar solution on the header elements?

@snide
Copy link
Contributor

snide commented Oct 14, 2019

I'll be adding a PR to fix some screen reader bits and CSS fixes.

To the multiple interactive elements issue, I'll point out this scenario should be EXTREMELY rare, and I think it's OK to let that part pass for the moment and just put a warning up in the docs. The main need for this functionality is so that people can add a clickable "column menu popup" into the header. These should always be single buttons, which themselves invoke a focus trapped popover. I can't think of a good reason anyone would want multiple buttons inside of a column header.

@thompsongl
Copy link
Contributor

@snide Agreed on the rarity bit. If this is a thing we can solve with design documentation, I'd almost prefer that over introducing a focus trap.

@myasonik
Copy link
Contributor

How would y'all feel about erroring if we detect multiple tabbable header elements?

Because we don't expect implementing developers to do this and it's not a good experience for end users, if we throw an error, then an implementing dev will have to at least bring it up to us so we know it's a real use case (or, we can suggest an alternative).

@myasonik
Copy link
Contributor

@chandlerprall, small bug:
If the header cells are focusable, the first time a user tabs into the grid, it should place focus on the first header cell, not on the first grid-body cell.

@chandlerprall
Copy link
Contributor Author

If the header cells are focusable, the first time a user tabs into the grid, it should place focus on the first header cell, not on the first grid-body cell.

Fixed!

@chandlerprall chandlerprall mentioned this pull request Oct 15, 2019
10 tasks
@chandlerprall
Copy link
Contributor Author

Added a console.warn when more than one tabbable element is discovered in a single header cell.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

New additions LGTM

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Pushed some design fixes (I'll flesh out a popover example in the docs PR), and fixed some accessibility stuff.

@myasonik
Copy link
Contributor

Found a bug 😕

Pick a sort, then remove that sort:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.

@chandlerprall
Copy link
Contributor Author

Found a bug 😕
Pick a sort, then remove that sort:

This only happens when the sort is added and removed without interacting with other elements. I took a quick look and was unable to immediately find the cause. As this doesn't have any meaningful impact I think we're good to merge with this state and make a deeper investigation separately.

@chandlerprall chandlerprall merged commit 33b7080 into elastic:feature/euidatagrid Oct 16, 2019
@chandlerprall chandlerprall deleted the feature/euidatagrid-column-headers branch October 16, 2019 16:13
chandlerprall added a commit that referenced this pull request Oct 24, 2019
* initial datagrid

* protect against re-rendering content on column width change

* Added unit initial tests for EuiDataGrid

* [EuiDataGrid] Base layer Sass (#2171)

* Initial css fixes

* works in IE, addresses feedback

* remove user select

* Adds basic aria roles and grid navigation (#2187)

* Adds basic aria roles and grid navigation

Co-Authored-By: Chandler Prall <chandler.prall@gmail.com>

* [Data Grid] Grid style options (#2176)

Adds basic styling to the data grid.

* Add pagination to EuiDataGrid (#2188)

* Added pagination to to EuiDataGrid

* Move EuiDataGrid row rendering to a sub-component to clean up state management

* EuiDataGrid pagination unit tests

* fix data grid pagination

* revert colors

* EuiDataGrid column visibility & ordering (#2207)

* Show/hide and re-order datagrid columns

* Column visability & ordering tests

* column styling

* column sizing and bars

* blergh

* tests

* feedback

* Fix linting

* Adds more complex focus control for DataGrid (#2222)

* [Data Grid] - Styling built into data grid, full screen mode (#2230)

Styling built into data grid, full screen mode

* EuiDataGrid add column sorting (#2278)

* API interface for providing column sort order, callback to allow external data sorting

* EuiDataGrid renders content into memory, sorts on it

* Added tests for EuiDataGrid sorting

* Added aria-sort value to a singly-sorted column header

* small cleanup

* add tests back in, though they are still broken

* Clean up some keyboard navigation issues

* Fix column sorting & update snapshots

* EuiDataGrid hooks cleanup (#2331)

* Refactored EuiDataGrid's hooks

* Fix datagrid to react to gridStyle changes

* [EuiDataGrid] Automatic column schema detection (#2351)

* Automatically detect data schema for in-memory datagrid

* Merge in described schema for field formatting

* Better column type detection

* Tests for euidatagrid schema / column type

* refactor datagrid schema code, add datetime type detection

* some comments

* Allow extra type detectors for EuiDataGrid

* cleanup of docs and type formatting

* Fix datagrid unit test

* Update currency detector

* Allow EuiDataGrid's inMemory prop to be {true}

* Added ability to provide extra props for the containing cell div

* Added test for cell props

* [EuiDataGrid] InMemory Performance (#2374)

* Automatically detect data schema for in-memory datagrid

* Merge in described schema for field formatting

* Better column type detection

* Tests for euidatagrid schema / column type

* refactor datagrid schema code, add datetime type detection

* some comments

* Allow extra type detectors for EuiDataGrid

* cleanup of docs and type formatting

* Fix datagrid unit test

* Update currency detector

* Allow EuiDataGrid's inMemory prop to be {true}

* Added ability to provide extra props for the containing cell div

* Added test for cell props

* Performance cleanups

* Clean up datagrid doc's inMemory selection

* Merged in feature branch

* EuiDataGrid in-memory options

* Performance refactor for in-memory values

* added a comment

* Fix sorting on in-memory and schema datagrid docs

* [EuiDataGrid] Feature/euidatagrid menu ux (#2392)

Moved the sorting mechanism to the top toolbar.

* Export useRenderToText to top-level package (#2412)

* [DATA GRID] Expand cells (#2418)

Data grid cells now can expand and can render individually based upon their schema.

* [EuiDataGrid] use schema information when sorting (#2419)

* cell expansion working mostly

* fix double import

* add search to field selector

* euitext

* cell epansion is now optional through a config

* keydown event for cells

* remove tabbables

* Clean up some code & tests

* Remove unused line of code

* Center popover against cell

* Update euidatagridcell popover placement, trigger, dom structure, and auto focusing

* Restore focus to grid cell when popover was in response to mouse click

* Allow grid column selection to be searchable

* Refactor expansion popover formatting, allow custom ones

* schema-based sort comparators

* reverse boolean sort to be true-false

* adds json schema sorting, fixes issue with popover

* Weaken the currency type detector when values have a period in their first few characters, and fix test

* Move column order and visibility to be managed externally (#2422)

* fix tests

* [EuiDataGrid] Custom column headers (#2421)

* Allow custom ReactNode for column header display

* Allow navigation into grid headers if any are interactive

* Properly wrap cell focus and use [enter], [f2] to interact

* Corrected header cell focus-state on blurring, [escape]. and single interactives

* Corrected header cell focus-state on blurring, [escape]. and single interactives

* When datagrid header is interactive, default its tabstop to the first header cell

* EuiDataGridHeaderCell warns about multiple interactive elements

* fix focus, example and screenreader stuffs, looks like tests pass

* simplifying screen reader read out

* [DATA GRID] EuiGridToolBar toolbar is now configurable through props (#2443)

* EuiGridToolBar toolbar is now configurable through props

* better tests

* small test typp

* Update src/components/datagrid/data_grid_types.ts

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* feedback

* [EuiDataGrid] Docs and autodocs (#2449)

* Render out EuiDataGrid proptypes

* Add pagination props to docs

* Fill out all datagrid autodoc sections

* remove debugger statement

* Update src/components/datagrid/data_grid_types.ts

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* words

* docs start

* datatype renamed to schema, update docs

* docs, fix typo for fullscreen buton

* core concepts

* better in memory explanation

* custom schema example

* provide a nice, documented snippet

* typos

* don't show pagination when only one page

* clean up styling, better docs for formatters

* more docs cleanup

* IE fix

* IE fix again

* small cleanup of docs

* describe how to disable expansion popovers

* dark mode tweaks

* Fix custom datatype sorting

* Update src-docs/src/views/datagrid/datagrid_example.js

Co-Authored-By: Michail Yasonik <michail@yasonik.com>

* Update src-docs/src/views/datagrid/datagrid_example.js

Co-Authored-By: Michail Yasonik <michail@yasonik.com>

* Update src-docs/src/views/datagrid/datagrid_example.js

Co-Authored-By: Michail Yasonik <michail@yasonik.com>

* Update src-docs/src/views/datagrid/datagrid_example.js

Co-Authored-By: Michail Yasonik <michail@yasonik.com>

* Update src-docs/src/views/datagrid/datagrid_example.js

Co-Authored-By: Michail Yasonik <michail@yasonik.com>

* Update src-docs/src/views/datagrid/datagrid_example.js

Co-Authored-By: Michail Yasonik <michail@yasonik.com>

* Update src-docs/src/views/datagrid/datagrid_example.js

Co-Authored-By: Michail Yasonik <michail@yasonik.com>

* Update src-docs/src/views/datagrid/datagrid_example.js

Co-Authored-By: Michail Yasonik <michail@yasonik.com>

* PR feedback

* typo

* feedback to break up docs

* better cross linking and summary

* fix custom schema display

* Update src-docs/src/views/datagrid/datagrid_memory_example.js

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* Update src-docs/src/views/datagrid/datagrid_memory_example.js

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* Update src-docs/src/views/datagrid/datagrid_memory_example.js

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* Update src-docs/src/views/datagrid/datagrid_schema_example.js

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* Update src-docs/src/views/datagrid/datagrid_memory_example.js

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* Update src-docs/src/views/datagrid/datagrid_memory_example.js

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* Update src-docs/src/views/datagrid/datagrid_memory_example.js

Co-Authored-By: Greg Thompson <thompsongl@users.noreply.github.com>

* Updated some datagrid docs

* main dg example page feedback

* Eui prefix all the things to be consistant. Adjust the data grid docs to match

* rewrite intro based on feedback

* more tweaking of words

* rename toolbarDisplay->toolbarVisibility

* in memory docs reworked to four examples

* clean up core example

* data grid styling snippets

* fix prop list

* Minor grammar edits

* Added isDetails prop to renderCellValue, reducing the use case for expansionFormatters. Speaking of those, expansionFormatter(s) has been renamed to popoverContent(s) and now recieve the rendered cell div in addition to the renderCellValue ReactElement

* fix docs renaming, fix css

* last docs edit seems fitting

* somewhat decent attempt at putting classnames on schemas

* Revert "somewhat decent attempt at putting classnames on schemas"

This reverts commit 26542d7.

* changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants