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 BulkSelect component #146

Merged
merged 7 commits into from
Jun 11, 2024
Merged

Add BulkSelect component #146

merged 7 commits into from
Jun 11, 2024

Conversation

fhlavac
Copy link
Collaborator

@fhlavac fhlavac commented Jun 7, 2024

RHCLOUD-33144

Added BulkSelect component to component groups with docs and tests

screencapture-localhost-8006-extensions-component-groups-bulk-select-2024-06-07-15_14_27

@fhlavac fhlavac marked this pull request as ready for review June 7, 2024 13:14
@patternfly-build
Copy link

patternfly-build commented Jun 7, 2024


### Bulk select with all option

To display an option for seleting all data at once, pass `canSelectAll` flag together with `totalCount` of data entries. You can also remove the page select option by setting `isDataPaginated` to `false`,

Choose a reason for hiding this comment

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

just a typo on seleting (selecting)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andrew-ronaldson fixed, thank you 🙂

splitButtonOptions={{
items: [
<MenuToggleCheckbox
ouiaId={`${ouiaId}-checkbox`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we might want to provide some sort of MenuToggleCheckboxProps. We can omit the onChange and isChecked props, but the rest should be configurable.

items: [
<MenuToggleCheckbox
ouiaId={`${ouiaId}-checkbox`}
id={BulkSelectValue.page}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't use constant like this. There will be ID errors with multiple bulk selects on a page.

@fhlavac fhlavac force-pushed the bulk branch 3 times, most recently from 789c971 to 6674c11 Compare June 10, 2024 11:08
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

I notice that when you use a keyboard to select an item from the menu, the focus then gets lost. I believe the focus should return to the toggle after an item is selected.

@fhlavac
Copy link
Collaborator Author

fhlavac commented Jun 11, 2024

@nicolethoen Oh yeah, I've added the PF flag for that thing. The problem is that it does not work for the splitButtonOptions. As far as I see correctly in the PF MenuToggle implementation, the ref is being passed to the div element for the split variant, not the button as it is in the case of regular and typeahead variants. Perhaps we should open an issue for it on the PatternFly core side and after resolving it, the focus should start working properly. What do you think?
Snímek obrazovky 2024-06-11 v 16 08 07

@fhlavac fhlavac requested a review from nicolethoen June 11, 2024 14:24
@fhlavac
Copy link
Collaborator Author

fhlavac commented Jun 11, 2024

@Hyperkid123 @andrew-ronaldson @nicolethoen do you think we can get it to pre-release as it is, so we can start using it in DataView development? The focus thing will be fixed as a follow-up depending on the response from PF devs.

@fhlavac fhlavac merged commit 0e40ad7 into patternfly:main Jun 11, 2024
6 checks passed
@fhlavac fhlavac deleted the bulk branch June 14, 2024 11:35
nicolethoen pushed a commit to fhlavac/react-component-groups that referenced this pull request Jun 17, 2024
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.

5 participants