-
Notifications
You must be signed in to change notification settings - Fork 357
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
feat(DataList): add draggable feature for basic list #4852
Conversation
PF4 preview: https://patternfly-react-pr-4852.surge.sh |
Re: changes in DataListCheck. The So it's not breaking currently, but a flag will have to be passed to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kmcfaul This is looking good. Just 2 problems that I'm seeing. On Firefox, there seems to be some problem with dropping the item. It sometimes is staying in the disabled state after I release the mouse. Have you tested on Firefox? Also, the cursor behavior is a bit flaky. As I drag the item, the cursor may update as I'm moving over other items below it.
@mcarrano Firefox just wanted the As far as the pointer, I would guess it is because the dragging doesn't interfere with css below it. I'm not sure if there's anything I can do about that, but I'll look into it. Any thoughts @mcoker ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This is looking great so far! The keyboard interaction works pretty well (though I wonder if we should include enter as one of the activation key presses like this demo), but I think there's some a11y updates that we should add to make it accessible for screen readers. Jenn and I hashed out all of the details in the core PR starting here, but essentially I think a structure like the core example would be great, potentially including the keyboard interaction instructions above the example as they did as well:
Some notable points from it:
- More a11y on the button:
aria-label
with "Reorder" telling users what they can do,aria-pressed
with true/false depending on whether the button is selected,aria-labelledby
the item,aria-describedby
with the description text of how to use the reorder button. aria-live="assertive"
as a div on the page that announces what is happening for users with the screen reader class.- I also notice that the checkboxes are labelled by an id that doesn't exist? It looks like the id of the list item is "simple-item1" instead of "check-item1".
(I think making these changes should also fix the axe violations happening. 🙂)
@kmcfaul when I test on Firefox, the row I'm dragging is really small. Are you seeing that? I'm also seeing the cursor issue. I can't see what's happening with the row in the inspector when I drag it. Can you describe what happens to the row that follows the cursor as you drag it over the data list? Is it cloned and absolutely positioned? I think we should add a class to the row that's being dragged and assign pointer styles to it. I would also like to assign a |
@mcoker As far as I can tell, the drag image is a literal image of the row, positioned on the cursor. I don't think the cursor sees it at all? Everything it is dragging over would have to have that pointer styling (or maybe it's possible to programmatically set the styling on the body while it's dragging, and remove it when the drag ends/is cancelled). The drag image is part of HTML 5 handler events so browsers may handle it slightly differently. Setting a custom drag image is possible, but it's expecting an image/simple element, not a React Element / clone of something. |
@jessiehuff The check aria-labelledby I think I just misunderstood when I was editing examples, it should probably be labelled by the items' ids. I'm having trouble understanding how edit: Updating with what I've added currently minus the live tag. |
@kmcfaul ah, gotcha! in that case we should be able to do something like this? https://codepen.io/mcoker/pen/oNxRRbe |
That might work. Do we need to add a new class for it or can it be detected via the |
The ghost row class is added to the clone that we move around in the data-list as you drag right? This class would be added to the thing the browser creates when you drag. You can test it by adding |
Codecov Report
@@ Coverage Diff @@
## master #4852 +/- ##
==========================================
- Coverage 52.49% 52.14% -0.35%
==========================================
Files 527 529 +2
Lines 9603 9701 +98
Branches 3552 3572 +20
==========================================
+ Hits 5041 5059 +18
- Misses 3916 3993 +77
- Partials 646 649 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Hmm that doesn't seem to work ( From what I've looked up so far, it could be the text selection in Chrome just overrides the pointer? FF doesn't seem to have this issue either. |
Just popping in here to say, @kmcfaul this is really cool! I'll take my time walking through to review it shortly, but playing with it is so fun! |
I completely understand what you mean about the
I agree with Nicole though, this looks awesome!!! 😄 |
@jessiehuff Added the aria-live div and support for updating its text, let me know if I approached this the right way! I also added logic for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit in terms of the string used in dragging the items, but otherwise looking great!!! (It currently uses the index- i.e. "Dragging index 0 to 1", but I'm not sure it's completely ideal since it dynamically changes during drag? Perhaps we could just be more generalized and say "Dragging Item 1". Other than that, the keyboard interaction is awesome, and I think it works in VO (it's slightly tough to test because of the previous role that was added ("listbox") which is interfering with testing (removing in #4913). VO seems to really not like nesting form elements inside of other form elements like buttons inside of list boxes. I can pull it down though and check locally.
7a96525
to
23b951d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We can work on the cursor styles in the next sprint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in the draggable example, should I be able to check a checkbox by hitting enter when focusing on the box? I dont think that's working.
Rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It still works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks for all the work on this @kmcfaul .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The biggest issue I see is that the Esc key no longer works in stopping the action. This makes it not a great user experience and rather confusing for keyboard users. You can tab out of it so it's not a keyboard trap from what I can tell (which is good), but it has some odd behavior and styling in this scenario:
-
Some minor things that could be fixed are that because the ids on the page are not unique, when testing with screen reader, it thinks it's labelled by previous examples so the VO output is odd. Not urgent though as it reads fine when isolated by itself.
-
We can open another issue as I don't want to hold anything up. However, the Esc key is a pretty big issue that we would need to prioritize.
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #4708
Basic implementation of draggable. DataLists with nested state dependencies (like expandable) I am having trouble getting to work, because the whole list needs to be saved in the state in order to update, but you cannot access state variables inside of itself.
@mcoker The draggable ghost defaults to a copy of the whole row, and I haven't found a great way to change this yet, but it at least seems to preserve the spacing. The entire row is also draggable by default instead of just the chip button (with mouse), but it doesn't interfere with other actions so I'm not sure this is an issue.
For keyboard, hitting
space
orenter
on a drag chip button begins the drag. Use theup arrow
ordown arrow
to move the row (showing the ghost placeholder) andenter
to confirm/finish the drag.Escape
will cancel the drag, or hitting any other key (likeTab
to continue tabbing through the DOM).