feat(Table): add draggable table rows demo#5921
Conversation
|
@nicolethoen The onClick handler for the draggable cell isn't needed for the draggable events at this time. The problem with putting the event handlers on the drag button instead of the whole row is that the 'ghost' you see when dragging would be incorrect (seeing only the button) and the re-ordering gets a bit wonky. I think we could update this later, maybe after revisiting the draggable implementation overall. But right now the whole row is getting dragged like in DataList. I don't think that should interfere with any control buttons in the row. |
|
I know in my code i deleted all the other examples - I don't think you should do that ;) |
|
Oh that would be bad haha. I didn't notice that. Will update the PR |
|
Updated, hopefully. Rolled back to the commit before your initial commit from the file history then re-applied the draggable table example. |
|
surge site?: patternfly-react-add_draggable_table.surge.sh |
Yeah that does work. I'm thinking it's maybe because the branch is on the PF repo already but I'm unsure. Regardless, I'll direct link to the example at the top of the PR. |
| variant="plain" | ||
| className={className} | ||
| type="button" | ||
| aria-label={ariaLabel || `Draggable row draggable button`} |
There was a problem hiding this comment.
@jessiehuff does this make sense for default aria-label?
There was a problem hiding this comment.
Jessie is out - can you think of an alternative?
There was a problem hiding this comment.
For what its worth, DataList has no default aria-label, but its example code uses "Reorder" as it's aria-label, paired with aria-labelledby with the list item id.
Edit: It also uses aria-describedby and aria-pressed but those are more tied to the keyboard interaction, which isn't in for table yet.
There was a problem hiding this comment.
I think it is ok to leave as is and get Jessie's feedback once she returns.
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
This looks great! Though the parent <td> for the draggable button needs the class .pf-c-table__draggable. That will fix the button's spacing and cursor.
And core is missing a background color on the ghost row, so I put up a PR for that - patternfly/patternfly#4159
mcarrano
left a comment
mcarrano
left a comment
There was a problem hiding this comment.
This looks good @kmcfaul . I wasn't completely clear about the discussion around whether to allow the whole row of just the drag handle to be draggable. I like how this works now for this example, but could see where we might get into trouble if there were other active elements in the row. Did you say this is or isn't consistent with how data list works? I guess that the issues would be the same.
|
It is the same as DataList |
|
Waiting on an additional core css class for drag-over styling |
kmcfaul
dismissed stale reviews from dlabrecq, nicolethoen, tlabaj, and mcarrano
via
ab4d305
kmcfaul
force-pushed
the
add_draggable_table
branch
from
42e9bea to
ab4d305
Compare
|
@mcoker Updated with the new class. The |
mcoker
left a comment
mcoker
left a comment
There was a problem hiding this comment.
L🔥TM!!! Fantastic work on getting this in so quickly!! 🏆🥇
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
8 participants
What: Closes #5817
Basic dragging functionality. Keyboard interaction may have to be added in a follow-up.
@nicolethoen
Surge direct link: https://patternfly-react-add_draggable_table.surge.sh/components/table/#composable-draggable-table