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

feat(DualListSelector): add disabled flag #6442

Merged
merged 3 commits into from
Oct 20, 2021

Conversation

jcaianirh
Copy link
Contributor

What: Closes #6250

Additional issues:
Added an isDisabled flag to the DualListSelector, and added an example to the examples page with a checkbox to enable/disable the control.
Added unit tests, demo-app entry, and cypress integration test.

@patternfly-build
Copy link
Contributor

patternfly-build commented Oct 13, 2021

@jcaianirh
Copy link
Contributor Author

@jessiehuff curious if this would also need some accessibility props

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

@jcaianirh If isDisabled is set, I would expect menu items to behave like other disabled menu items (see Dropdown or Menu with disabled items, for example). Looks like those styles are available. See this Core example: https://staging.patternfly.org/v4/components/dual-list-selector/html/#tree-view-with-chosen-and-disabled-options

@jcaianirh
Copy link
Contributor Author

@mcarrano utilized core styles for the items. @tlabaj @nicolethoen removed integration tests and demo app entry.

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @jcaianirh !

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

I'm noticing that with VO, the dual list selector items still seem available through screen reader. I'm thinking that we should add aria-disabled="true" when the dual list selector is disabled on the <ul> which will announce all of its contents as disabled as well.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

Looks good other than the comment Jessie left.

@jcaianirh
Copy link
Contributor Author

@tlabaj @jessiehuff added aria-disabled prop to the ul

Copy link
Contributor

@jessiehuff jessiehuff left a comment

Choose a reason for hiding this comment

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

LGTM! :)

@tlabaj tlabaj merged commit 85e1314 into patternfly:main Oct 20, 2021
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.2.0
  • @patternfly/react-catalog-view-extension@4.14.0
  • @patternfly/react-charts@6.16.0
  • @patternfly/react-code-editor@4.4.0
  • @patternfly/react-console@4.14.0
  • @patternfly/react-core@4.163.0
  • @patternfly/react-docs@5.24.0
  • @patternfly/react-icons@4.14.0
  • @patternfly/react-inline-edit-extension@4.8.0
  • demo-app-ts@4.123.0
  • @patternfly/react-integration@4.125.0
  • @patternfly/react-log-viewer@4.8.0
  • @patternfly/react-styles@4.13.0
  • @patternfly/react-table@4.32.0
  • @patternfly/react-tokens@4.15.0
  • @patternfly/react-topology@4.10.0
  • @patternfly/react-virtualized-extension@4.10.0
  • transformer-cjs-imports@4.1.0

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DualListSelector: add isDisabled prop
8 participants