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

Document EuiAccordion disabled state #6088

Closed

Conversation

cjcenizal
Copy link
Contributor

Summary

image

I'm anticipating the need to disable an accordion in elastic/kibana#137464 and found that this state seems to be supported by the existing props, but isn't documented. I added some documentation for this state and would love some guidance on whether this is indeed supported with the existing props, and if so how to improve the docs for it.

There are a couple weird things about this state that makes me suspect we didn't intend to support it originally.

Incomplete hover state. The hover state for the .euiAccordionForm__button class doesn't take into account a disabled state, so even though you can't click it, it still looks clickable when you hover.

TS errors. There are TS errors when I pass arrowProps and buttonProps like this:

<EuiAccordion
  id={disabledAccordionId}
  buttonContent="Security settings"
  arrowProps={{ disabled: true }}
  buttonProps={{ disabled: true }}
  initialIsOpen={true}
>

arrowProps error:

src-docs/src/views/accordion/accordion_disabled.tsx:14:23 - error TS2322: Type '{ disabled: true; }' is not assignable to type 'Partial<Omit<EuiButtonIconProps, "aria-labelledby" | "onClick" | "iconType">>'.
  Object literal may only specify known properties, but 'disabled' does not exist in type 'Partial<Omit<EuiButtonIconProps, "aria-labelledby" | "onClick" | "iconType">>'. Did you mean to write 'isDisabled'?

buttonProps error:

src-docs/src/views/accordion/accordion_disabled.tsx:15:24 - error TS2322: Type '{ disabled: true; }' is not assignable to type 'CommonProps & HTMLAttributes<HTMLElement>'.
  Object literal may only specify known properties, and 'disabled' does not exist in type 'CommonProps & HTMLAttributes<HTMLElement>'.

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@cjcenizal cjcenizal added the documentation Issues or PRs that only affect documentation - will not need changelog entries label Jul 28, 2022
@cjcenizal cjcenizal force-pushed the feature/accordion-disabled-state branch from dd22055 to 5a1b4a1 Compare July 28, 2022 17:04
Comment on lines +451 to +453
playground: accordionConfig,
props: { EuiAccordion },
snippet: `<EuiAccordion
Copy link
Contributor Author

@cjcenizal cjcenizal Jul 28, 2022

Choose a reason for hiding this comment

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

I'd also appreciate guidance on what to do with these properties ln 451-453. 🙇

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_6088/

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

I added a comment for screen reader user case. LMK if you'd like to discuss further!

<div>
<EuiAccordion
id={disabledAccordionId}
buttonContent="Security settings"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see this buttonContent prop expanded to include a conditional EuiScreenReaderOnly component that further explains the disabled state. If I'm using a screen reader and listening to the Form Controls menu, I'm not going to have any of the error state context. The only thing I will hear is the label "Security settings, dimmed button."

This signals the button is there, but not why it's dimmed. A better message might be "Security settings has errors in expanded content. dimmed button." This isn't exact wording, but a for instance.

The EUI docs show buttonContent as a React Node, so you should be able to do something like the following snippet, assuming a new prop isDisabled:

+ const disabledButtonContent =
+  <>
+    <span>Security settings</span>
+    <EuiScreenReaderOnly>
+      <span>{' '}has errors in expanded content.</span>
+    </EuiScreenReaderOnly>
+  </>;

- buttonContent="Security settings"
+ buttonContent={isDisabled ? disabledButtonContent : "Security settings"}

Screen Shot 2022-07-28 at 12 20 09 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

@1Copenut If we were to build isDisabled directly into EuiAccordion, do you have any other advice we should be requiring of the consumer for the disabled state?

Copy link
Contributor

Choose a reason for hiding this comment

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

Off the top of my head the only other recommendation would be to only use isDisabled with Accordions that render a button on the page. Having a disabled attribute on a fieldset will thrown an error in axe (linter, unit tests, et al.)

@cchaos cchaos self-assigned this Jul 28, 2022
@cchaos
Copy link
Contributor

cchaos commented Jul 28, 2022

@cjcenizal While you are correct that it's possible to create a disabled version of EuiAccordion using these props, we really should just support isDisabled at the top level. The buttonProps prop doesn't accept disabled because it can either be a <button> or a <fieldset> depending on usage (though I guess this is still debatable #5767) and so the prop purely extends the generic HTMLElement properties.

Without adding some more complex TS to that prop and then having consumers have to create such a complicated setup, we should just support it at the top level. I'll take a quick look at this PR locally as a starting point since you've already created the docs section 🙇‍♀️ .

@cchaos
Copy link
Contributor

cchaos commented Aug 1, 2022

Closing in favor of #6095

@cchaos cchaos closed this Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues or PRs that only affect documentation - will not need changelog entries skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants