feat(table): convert column mgmt demos to fullscreen #7446
Conversation
|
Preview: https://patternfly-react-pr-7446.surge.sh A11y report: https://patternfly-react-pr-7446-a11y.surge.sh |
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
@jenny-s51 This looks great, but there seems to be a bug with column reordering via drag & drop. If I reorder columns and then click Save, upon returning to the table, it's empty.
jenny-s51
left a comment
jenny-s51
left a comment
There was a problem hiding this comment.
Great catch @mcarrano ! I've been thoroughly investigating here; this is not a trivial bug to fix, so I opened #7471 to address this issue as part of a necessary refactor.
Our demo is using a deprecated API on DataList that implemented the drag/drop functionality. It would make more sense to refactor the code here in favor of the new DragDrop component, which will likely fix this bug and also keep us up to date with how we currently demo DragDrop in DataList! WDYT @tlabaj ?
|
Looks good, apart from what @mcarrano pointed out! Another thing, that maybe is outside the scope of this issue, is that I expected that while I'm dragging a row around, the other rows would move to "make room" for the dragged row, so that I would be able to have a visual of what i'm doing before I drop the row where I want it. Idk if that makes sense, but I just feel like it's hard to tell where the row is going to land while i'm dragging it..the same way the draggable Data list demo works! |
|
Thank you @mmenestr, good point - the visual look of the current drag/drop implementation here is different from the drag/drop visuals that we see in the DataList draggable demo. This is also due to the newer The dragging visuals will be addressed separately in #7471 when this code is refactored to use |
@mcarrano @jenny-s51, I agree, we would want to suggest refactoring to use new drag and drop. |
|
kk sounds good! Just tried it out again, and the drag and drop is working as expected now!! But one thing I noticed is that when I go back into the manage columns modal after I've re-ordered something, the order that the columns show up in does not match the order they're in inside the table. Shouldn't the order of the columns in the modal, match the order of the columns in the table? |
mmenestr
left a comment
mmenestr
left a comment
There was a problem hiding this comment.
Thanks @jenny-s51 In that case, LGTM!
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
Had a couple of nits below, but I'm also noticing an issue with both of the examples updated. The Pagination looks to be breaking outside of the main page content when I scroll to the very bottom of the page:
versus how the bulk select demo looks:
| <Toolbar id="page-layout-table-column-management-action-toolbar-top">{toolbarItems}</Toolbar> | ||
| </React.Fragment> | ||
| } | ||
| aria-label="This is a table with checkboxes" |
There was a problem hiding this comment.
| aria-label="This is a table with checkboxes" | |
| aria-label="This is a table with draggable column management" |
| <Toolbar id="page-layout-table-column-management-action-toolbar-top">{toolbarItems}</Toolbar> | ||
| </React.Fragment> | ||
| } | ||
| aria-label="This is a table with checkboxes" |
There was a problem hiding this comment.
| aria-label="This is a table with checkboxes" | |
| aria-label="This is a table with column management" |
|
Further progress on this issue is currently blocked, as it is dependent on the Drag and Drop refactor. Once the Drag and Drop refactor is complete, the demo will to be updated with the new DnD implementation, which will resolve the existing issues/bugs noted in review comments (see above). Per @nicolethoen's suggestion , I am closing this issue and opening a separate one to track the remaining work, which will include using the new table data here as part of the epic #7384 , and pulling in these updates to make the demo fullscreen! |
6 participants
What: Closes #7357