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.

Sign up for GitHub

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

Jump to bottom

[EuiDataGrid] Enforce a cell popover if cell actions are present #5710

Merged
merged 5 commits into from
Mar 17, 2022
Merged

[EuiDataGrid] Enforce a cell popover if cell actions are present #5710

merged 5 commits into from
Mar 17, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 10, 2022 •
edited
Loading

Summary

This is essentially the cherry-picked second half of #5675, along with several misc setup/cleanup commits that I grabbed just to get in now (follow along by commit to review them).

Since the first half of #5675 is on hold due to design decisions/discussion, I figured we might as well get this fix+cleanup now, since it potentially affects accessibility/keyboard users.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles

- [ ] Checked Code Sandbox works for any docs examples

  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cee-chen cee-chen requested a review from chandlerprall March 10, 2022 18:40
@cee-chen cee-chen marked this pull request as ready for review March 10, 2022 18:44
@cee-chen cee-chen force-pushed the datagrid/isexpandable-cellactions branch from df5656e to ae5a918 Compare March 10, 2022 18:52
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5710/

chandlerprall
chandlerprall suggested changes Mar 14, 2022
View reviewed changes
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.

Only comment is on the changelog entry being a breaking change, otherwise this LGTM after testing in the preview build

CHANGELOG.md Outdated Show resolved Hide resolved
cee-chen
cee-chen commented Mar 14, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@cee-chen cee-chen requested a review from chandlerprall March 14, 2022 20:22
@cchaos
Copy link
Contributor

cchaos commented Mar 14, 2022

Is it possible to also de-enforce cell expansion when the cell contents is empty? Like in a footer?
Screen Shot 2022-03-14 at 16 23 57 PM

@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 14, 2022 •
edited
Loading

Is it possible to also de-enforce cell expansion when the cell contents is empty? Like in a footer?

Unfortunately EuiDataGrid isn't really aware of the cell contents, it simply renders whatever is passed in renderCellValue as a component.

What we could do is change that specific footer example to hide isExpandable at the cell level (now that we support configuring that per-cell via setCellProps) and recommend other consumers do the same.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5710/

@cee-chen
Copy link
Contributor Author

@cchaos I'm going to open a separate PR for that docs example change as I found this additional hilarious footer-related bug at the same time

before

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5710/

@cee-chen cee-chen mentioned this pull request Mar 14, 2022
Merged
4 tasks
cee-chen added 4 commits March 15, 2022 13:45
@cee-chen
[misc setup] refactor cell actions unit tests to use mock component
ee65072 
- this allows us to `component.find()` by the 'MockAction' name, which future unit tests will be using to determine the # of actions rendered
@cee-chen
Misc docs cleanup
da8eb04 
- Add intro paragraph to cell popovers page, explaining the UX purpose of the expansion popover

- Fix incorrect cellActions example in main datagrid doc

- Fix variable casing on source code
@cee-chen
Force cell isExpandable to evaluate as true if cellActions exist
0ca49a8 
+ add more comments/documentation to note this behavior since I missed it the first time

+ simplify hasCellActions to just use isExpandable, now that isExpandable accounts for cellActions
@cee-chen
Add changelog entry
63cd8d2
@cee-chen cee-chen force-pushed the datagrid/isexpandable-cellactions branch from 814332f to 63cd8d2 Compare March 15, 2022 21:37
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5710/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5710/

@cee-chen cee-chen enabled auto-merge (squash) March 17, 2022 20:52
@cee-chen cee-chen merged commit 7d640c0 into elastic:main Mar 17, 2022
@cee-chen cee-chen deleted the datagrid/isexpandable-cellactions branch March 17, 2022 20:59
@breehall breehall mentioned this pull request Mar 22, 2022
Merged
@cee-chen cee-chen mentioned this pull request Mar 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chandlerprall chandlerprall chandlerprall approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cee-chen @kibanamachine @cchaos @chandlerprall