Added a "justify" to the ExpanderControl component to make the icon v… #6570
Conversation
…ertically centered and to handle non-standard themes or styling. Removed the "usingKeyboard", it conflicted with switching apps (at least on mac) using Cmd-Tab and rendered the top row focused/active independent on which rows was expanded. The only case I could find where this would be applicable would be using the "onSelect" use-case, but that still works as intended after manual testing. I would appreciate some input whether which is correct though.
| align="center" | ||
| fill | ||
| pad={pad} | ||
| justify="center" |
There was a problem hiding this comment.
I think there are cases where it is desirable to not have the expander icon vertically centered. For example if you have a DataTable with rows that take up a lot of vertical space, having the expander icon at the top of the row makes it easier to associate with the row (see screenshots below)
Expander icon with justify="center"
Expander icon without justify="center"
I understand a use case like this might not always be the case, but I'm just hesitating to make justify="center" the default.
Another note on this. It looks like verticalAlign is being passed through but isn't having an effect on the expander cell. This could be something to look into. Perhaps instead of making justify="center" the default we can utilize verticalAlign so that people can control this?
There was a problem hiding this comment.
Thanks for your feedback! Those screenshots definitively make it look like a poor idea! 🙈
I'll take a look into verticalAlign.
There was a problem hiding this comment.
I've updated the PR with your input. I would really like to remove fill on the Expander Button, I think it's ugly that it's filling the entire height, but I also think it wouldn't be appreciated to change the default behaviour... Overriding with theme setting possibly?
There was a problem hiding this comment.
Yeah I think a theme setting would make sense. Maybe dataTable.expand that can accept button props?
- Expander button now takes `verticalAlign` into account and not using justify. Updated storybook to show this. - "Restored" useKeyboard with the condition it should only be used if `onClickRow` is used (i.e. that is the only case when a row should get focus).
What does this PR do?
Fixing two visual bugs.
justify="center"to the ExpanderControl component to make the icon vertically centered and to handle non-standard themes or styling.use-keyboard.jswhich caused this effect. Removed the "usingKeyboard", the only case I could find where this would be applicable would be using theonSelectuse-case, but that still works as intended after manual testing. I would appreciate some input whether which is correct though.Where should the reviewer start?
src/components/DataTable/ExpanderCell.jsandsrc/components/DataTable/Body.jsare the only files changed except the updated snapshot.What testing has been done on this PR?
usingKeyboard, I might need help here to verify it's not affecting something I'm not aware of, I'm not too familiar with grommet.How should this be manually tested?
The storybooks should suffice.
Do Jest tests follow these best practices?
screenis used for querying.userEventis used in place offireEvent.asFragment()is used for snapshot testing.Any background context you want to provide?
What are the relevant issues?
This partially resolves #6344
Screenshots (if appropriate)
Do the grommet docs need to be updated?
No
Should this PR be mentioned in the release notes?
No
Is this change backwards compatible or is it a breaking change?
Backwards compatible