Skip to content
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

fix(drag-drop) : improves drop indicator positioning in draggable context #1538

Merged
merged 14 commits into from
Apr 26, 2023

Conversation

geotrev
Copy link
Contributor

@geotrev geotrev commented Apr 21, 2023

Detail

While enhancing the drag and drop pattern with DraggableList.DropIndicator, it became clear it wasn't fully compatible with CSS transform (commonly used in dnd libraries).

This PR does two things:

  • Improves DraggableList.DropIndicator's compatibility with CSS transform; the result is significantly less overhead, e.g. spacing offsets and tests
  • Enhances the pattern example to align more closely to use-cases Garden expects, including options to test:
    • Horizontal list
    • Drop indicator
    • Placeholder
    • Compact
    • Bare
    • Disabled

The only thing not covered in the pattern overhaul is showing danger state on Dropzone, but that'll be included in website docs.

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer) (no change)
  • 🌐 demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • ♿ tested for WCAG 2.1 AA accessibility compliance (no change)
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@geotrev geotrev self-assigned this Apr 21, 2023
@geotrev geotrev requested a review from a team as a code owner April 21, 2023 21:14
Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of observations on the pattern demo:

  • The drop indicator is not available with keyboard (non-mouse) interaction
  • In vertical mode, the drop indicator shows at the top of the list by default, but the item goes to the bottom when dropped

I'm not sure if these are working as expected or represent short-comings with dnd-kit. Depending on the outcome, I may have a follow-on comment/concern re: the WAI-ARIA approach with the drop indicator <li>.

@geotrev
Copy link
Contributor Author

geotrev commented Apr 24, 2023

@jzempel I believe it's related to dnd-kit but I'll look into it today. 👌🏻

Copy link
Member

@jzempel jzempel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular treatment that hasPlaceholder is supposed to have on the demo?

@geotrev
Copy link
Contributor Author

geotrev commented Apr 24, 2023

@jzempel hasPlaceholder should show a gray box where a draggable was before the drag started:

Screenshot 2023-04-24 at 4 50 11 PM

(in the above, "Corn" is being dragged)

@coveralls
Copy link

coveralls commented Apr 24, 2023

Coverage Status

Coverage: 96.144% (-0.04%) from 96.179% when pulling 3752f45 on george/drag-drop-patterns into 6733c1d on main.

…ne; adds drop indicator for keyboard navigation
@geotrev geotrev merged commit 693d805 into main Apr 26, 2023
@geotrev geotrev deleted the george/drag-drop-patterns branch April 26, 2023 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants