patternfly-react icon indicating copy to clipboard operation
patternfly-react copied to clipboard

feat(table): convert column mgmt demos to fullscreen

Open jenny-s51 opened this issue 3 years ago • 7 comments

What: Closes #7357

jenny-s51 avatar May 19 '22 17:05 jenny-s51

Preview: https://patternfly-react-pr-7446.surge.sh

A11y report: https://patternfly-react-pr-7446-a11y.surge.sh

patternfly-build avatar May 19 '22 17:05 patternfly-build

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!

mmenestr avatar May 26 '22 20:05 mmenestr

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 DragDrop component that's being used there!

The dragging visuals will be addressed separately in #7471 when this code is refactored to use DragDrop, which will align the appearance with the draggable Data list demo!

jenny-s51 avatar May 27 '22 15:05 jenny-s51

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 ?

@mcarrano @jenny-s51, I agree, we would want to suggest refactoring to use new drag and drop.

tlabaj avatar May 27 '22 16:05 tlabaj

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 avatar May 27 '22 17:05 mmenestr

@mmenestr that will also be addressed as part of the refactor in #7471! The way the rows in the modal are currently ordered/implemented can be fixed/updated once we use DragDrop here instead 👍

jenny-s51 avatar May 31 '22 19:05 jenny-s51

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

stale[bot] avatar Jul 31 '22 16:07 stale[bot]

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!

jenny-s51 avatar Sep 26 '22 14:09 jenny-s51