Skip to content

feat(list)!: Support moving between different lists via keyboard#10480

Merged
driskull merged 60 commits intodevfrom
dris0000/sort-dropdown
Oct 31, 2024
Merged

feat(list)!: Support moving between different lists via keyboard#10480
driskull merged 60 commits intodevfrom
dris0000/sort-dropdown

Conversation

@driskull
Copy link
Copy Markdown
Member

@driskull driskull commented Oct 3, 2024

Related Issue: #7537

Summary

BREAKING CHANGE: Modifies the component's keyboard sorting experience by using a dropdown menu to move list items. The ListItem dragSelected property and calciteListItemDragHandleChange event have been removed due to no longer being relevant.

@github-actions github-actions Bot added the enhancement Issues tied to a new feature or request. label Oct 3, 2024
*
* @deprecated use calciteListItemSortHandleOpen instead.
*/
@Event({ cancelable: false }) calciteListItemDragHandleChange: EventEmitter<void>;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

can I just remove this since its a breaking change already?

Copy link
Copy Markdown
Member

@jcfranco jcfranco Oct 29, 2024

Choose a reason for hiding this comment

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

No, I'd like to keep this PR focused. Applies to other similar questions.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Apologies, I thought you were suggesting removing previously deprecated members. If the "keyboard list move" changes made the prop and event irrelevant, they should be fine to remove. Let’s just make sure to note this in the breaking change footer and our 3.x breaking change doc.

Copy link
Copy Markdown
Member

@geospatialem geospatialem left a comment

Choose a reason for hiding this comment

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

Overall this is great support, in particular when JAWS and NVDA are present. For both the keyboard keys and context provided is fantastic. ✨

Observed some odd behavior via keyboard (without AT active), where the "Reorder" menu seems to have a weird UX/behavior. Recorded a screen grab for the key combinations and context. I'm not sure if its something that could be mitigated, without impacting the AT experience, but wanted to showcase the "bugginess" experience:

testing-list-breaking-change

WDYT if we consider adding the ability to close the menu and shift focus back to the list-item when using the Esc key?

# Conflicts:
#	packages/calcite-components/src/components/list/list.tsx
@driskull
Copy link
Copy Markdown
Member Author

WDYT if we consider adding the ability to close the menu and shift focus back to the list-item when using the Esc key?

The calcite-dropdown is handling this escape action so it might be tricky to override it.

Observed some odd behavior via keyboard (without AT active), where the "Reorder" menu seems to have a weird UX/behavior. Recorded a screen grab for the key combinations and context. I'm not sure if its something that could be mitigated, without impacting the AT experience, but wanted to showcase the "bugginess" experience:

Can you test this again? What exactly is the expected behavior vs the observed behavior?

@geospatialem
Copy link
Copy Markdown
Member

WDYT if we consider adding the ability to close the menu and shift focus back to the list-item when using the Esc key?

The calcite-dropdown is handling this escape action so it might be tricky to override it.

Observed some odd behavior via keyboard (without AT active), where the "Reorder" menu seems to have a weird UX/behavior. Recorded a screen grab for the key combinations and context. I'm not sure if its something that could be mitigated, without impacting the AT experience, but wanted to showcase the "bugginess" experience:

Can you test this again? What exactly is the expected behavior vs the observed behavior?

The preventDefault() removal did the trick, it's performing as I'd expect now. 💪🏻

# Conflicts:
#	packages/calcite-components/src/components.d.ts
@driskull driskull marked this pull request as ready for review October 28, 2024 20:29
@driskull driskull requested a review from jcfranco October 28, 2024 20:29
Copy link
Copy Markdown
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

Had a few notes, but this is great stuff, @driskull!

I'd like to get @alisonailea to sign off on component tokens before proceeding, but in the meantime can you move the BREAKING CHANGE footer to the end of the description? It needs to be placed at the bottom to adhere to the conventional commits spec:

<type>[optional scope]: <description>

[optional body]

[optional footer(s)]

Once approved, this can be merged after the Oct milestone wraps up.

Comment thread packages/calcite-components/src/components/list-item/list-item.tsx
Comment thread packages/calcite-components/src/components/sort-handle/sort-handle.stories.ts Outdated
Comment thread packages/calcite-components/src/components/sort-handle/sort-handle.tsx Outdated
Comment thread packages/calcite-components/src/components/sort-handle/sort-handle.e2e.ts Outdated
Comment thread packages/calcite-components/src/components/sort-handle/sort-handle.e2e.ts Outdated
@driskull
Copy link
Copy Markdown
Member Author

@jcfranco @alisonailea I can just not document the tokens for now by removing the JS doc for them if that is what we want. This isn't a public component anyway.

@driskull driskull requested a review from benelan as a code owner October 29, 2024 17:50
@driskull
Copy link
Copy Markdown
Member Author

Removing tokens. for future reference here they are: ced0936

@driskull driskull added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Oct 31, 2024
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 31, 2024
@driskull driskull added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Oct 31, 2024
@driskull driskull merged commit 1005109 into dev Oct 31, 2024
@driskull driskull deleted the dris0000/sort-dropdown branch October 31, 2024 20:44
benelan pushed a commit that referenced this pull request Feb 8, 2025
)

**Related Issue:** #7537

## Summary

- depends on #10241
- updates `action` to allow cursor to be overriden
- lists should set the `label` property for the "Move to" sorting menu
to have a name for the list.
- adds `calcite-sort-handle` component to handle sorting and moving
between lists for non mouse users
  - internal component 
  - adds stories
  - adds tests 
- list-item
  - deprecates `dragSelected` property: no longer needed.
- deprecates `calciteListItemDragHandleChange` event. no longer needed.
- removed setting `aria-posinset` and `aria-setsize`. These are only
needed when only part of the items are availalbe in the DOM. So we can
safely remove these.
-
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-setsize
-
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-posinset
- replaces `calcite-handle` with `calcite-sort-handle` in the `list`
component.
- updates tests
- adds tests
- adds demo pages

BREAKING CHANGE: Modifies the component's keyboard sorting experience by
using a dropdown menu to move list items. The ListItem `dragSelected`
property and `calciteListItemDragHandleChange` event have been removed
due to no longer being relevant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Issues and pull requests with code changes that are not backwards compatible. enhancement Issues tied to a new feature or request. pr ready for visual snapshots Adding this label will run visual snapshot testing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants