chore(Table) bulk select demo cleanup#9095
Conversation
nicolethoen
force-pushed
the
table-bulkSelect-cleanup
branch
from
db6fdb2 to
fc0748d
Compare
|
Preview: https://patternfly-react-pr-9095.surge.sh A11y report: https://patternfly-react-pr-9095-a11y.surge.sh |
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
This is looking good @dominik-petrik ! Left some comments
| const [perPage, setPerPage] = React.useState(10); | ||
| const [paginatedRows, setPaginatedRows] = React.useState(rows.slice(0, 10)); | ||
| const [selectedRows, setSelectedRows] = React.useState([]); | ||
| const handleSetPage = (_evt, newPage, perPage, startIdx, endIdx) => { |
There was a problem hiding this comment.
| const handleSetPage = (_evt, newPage, perPage, startIdx, endIdx) => { | |
| const handleSetPage = (_evt, newPage, _perPage, startIdx, endIdx) => { |
| aria-label={anySelected ? 'Deselect all cards' : 'Select all cards'} | ||
| isChecked={isChecked} | ||
| onClick={() => { | ||
| anySelected ? setBulkSelection('none') : setBulkSelection('all'); |
There was a problem hiding this comment.
Looks like we want to update the checkbox logic here so it works correctly
| anySelected ? setBulkSelection('none') : setBulkSelection('all'); | |
| anySelected ? setSelectedRows([]) : selectAllRows(bulkSelection !== 'all'); |
| <Toolbar> | ||
| <ToolbarContent> | ||
| <ToolbarGroup> | ||
| <ToolbarItem variant="bulk-select">{buildBulkSelect()}</ToolbarItem> |
There was a problem hiding this comment.
Nit - can we specify in the name here that this is a dropdown?
| <ToolbarItem variant="bulk-select">{buildBulkSelect()}</ToolbarItem> | |
| <ToolbarItem variant="bulk-select">{buildBulkSelectDropdown()}</ToolbarItem> |
| /> | ||
| ); | ||
|
|
||
| const buildBulkSelect = () => { |
There was a problem hiding this comment.
Nit - can we specify in the name here that this is a dropdown?
| const buildBulkSelect = () => { | |
| const buildBulkSelectDropdown = () => { |
| import { rows, columns } from '../../examples/Data.jsx'; | ||
|
|
||
| export const BulkSelectTableDemo = () => { | ||
| const [isBulkSelectOpen, setIsBulkSelectOpen] = React.useState(false); |
There was a problem hiding this comment.
Nit
| const [isBulkSelectOpen, setIsBulkSelectOpen] = React.useState(false); | |
| const [isBulkSelectDropdownOpen, setIsBulkSelectDropdownOpen] = React.useState(false); |
| <Dropdown | ||
| role="menu" | ||
| onSelect={(_ev, itemId) => { | ||
| console.log(itemId); |
There was a problem hiding this comment.
| console.log(itemId); |
kmcfaul
left a comment
kmcfaul
left a comment
There was a problem hiding this comment.
Looks good, just needs that small logic update to handle the split dropdown checkbox click properly. I agree with the other nits as well, but not a blocker worst case.
6 participants
What: Closes #8975