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

add "expand_all" toggle to datagrid header #8152

Merged
merged 12 commits into from
Sep 12, 2022

Conversation

hiaselhans
Copy link
Contributor

this adds a "collapse all" Button in the DataGridHeader

Behavior:

  • if one or more rows are expanded, the CollapseButton shows an opened state.
    • onClick collapses all shown rows
  • if all rows are collapsed onClick all rows are expanded

@hiaselhans
Copy link
Contributor Author

demo: storybook

Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for that.

Would you mind adding a unit test based on the storybook?

packages/ra-core/src/controller/list/useExpanded.tsx Outdated Show resolved Hide resolved
packages/ra-core/src/controller/list/useExpanded.tsx Outdated Show resolved Hide resolved
@hiaselhans
Copy link
Contributor Author

thanks @fzaninotto !

I added the tests to cypress.
Hope that works, not sure how to add tests to the storybook...

@fzaninotto
Copy link
Member

For the tests, please write a react-testing-library test rather than an e2e test. You can leverage the Storybook as we did e.g. in https://github.com/marmelab/react-admin/blob/master/packages/ra-ui-materialui/src/field/ReferenceOneField.spec.tsx

@hiaselhans
Copy link
Contributor Author

ok, so i added expandAllButton.spec.tsx with best intentions. Hope that works now.
I would still not remove the e2e test, maybe it proves useful at some point

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Otherwise this looks good, thanks! 🙂

Copy link
Contributor Author

@hiaselhans hiaselhans left a comment

Choose a reason for hiding this comment

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

done! Thanks

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thanks!
I'll give some time to @fzaninotto if he wants to re-review it, then we'll be able to merge 🙂

packages/ra-core/src/controller/list/useExpanded.tsx Outdated Show resolved Hide resolved
packages/ra-core/src/controller/list/useExpanded.tsx Outdated Show resolved Hide resolved
packages/ra-core/src/controller/list/useExpanded.tsx Outdated Show resolved Hide resolved
: false;

const toggleExpanded = useCallback(() => {
const unaffectedIds = expandedIds.filter(
Copy link
Member

Choose a reason for hiding this comment

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

these are the notDisplayedExpandedIds, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as a compromise, i'd say unaffectedExpandedIds, since in this context we don't have displayed or hidden ids 👿

expanded ? 'ra.action.close' : 'ra.action.expand'
)}
aria-expanded={expanded}
tabIndex={-1}
Copy link
Member

Choose a reason for hiding this comment

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

Why this tabindex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dito

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since ExpandRowButtons are not tab-selectable, the least i would do here is to stay consistent and keep tabindex = -1.

: false;

const toggleExpandedAll = useCallback(() => {
const unaffectedExpandedIds = expandedIds.filter(
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this "unaffected".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

possible synonyms:

  • unChanged
  • unAltered
  • unTouched

@fzaninotto fzaninotto merged commit 4d5fb36 into marmelab:next Sep 12, 2022
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 4.4.0 milestone Sep 12, 2022
@hiaselhans
Copy link
Contributor Author

thanks! and keep up the good work! :)

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.

3 participants