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(accordion): add disabled state to accordion #7005

Merged
merged 3 commits into from
Oct 12, 2020

Conversation

tw15egan
Copy link
Member

@tw15egan tw15egan commented Oct 8, 2020

Closes #7000

Adds in disabled state for Accordion and AccordionItem

Changelog

New

  • disabled prop for Accordion and AccordionItem

Changed

  • Added two knobs to Accordion Playground, one to disable the entire Accordion, and the other to disable just one item.

Testing / Reviewing

Go to Accordion Playground and try disabling the entire Accordion as well as an AccordionItem

@netlify
Copy link

netlify bot commented Oct 8, 2020

Deploy preview for carbon-elements ready!

Built with commit 36edef6

https://deploy-preview-7005--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 8, 2020

Deploy preview for carbon-components-react ready!

Built with commit 36edef6

https://deploy-preview-7005--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Oct 8, 2020

Deploy preview for carbon-elements ready!

Built with commit f25afa8

https://deploy-preview-7005--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 8, 2020

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit f25afa8

https://deploy-preview-7005--carbon-components-react.netlify.app

Copy link
Contributor

@joshblack joshblack left a comment

Choose a reason for hiding this comment

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

For prop naming, what would be the best convention for went to decide between disabled and isDisabled?

One quick rule of thumb was that isDisabled is used for custom components and disabled is relegated to HTML elements but not sure how effective that is? 🤷

@tw15egan
Copy link
Member Author

tw15egan commented Oct 9, 2020

@joshblack essentially the disabled state here is being passed down to a button, so it is using the correct syntax. Just needed a modifier class to handle disabling the borders, since that is placed on the parent element. I'd prefer to just keep it disabled across the board, so we don't make users think "Is this one isDisabled or disabled?".

I did wonder about other props like light vs isLight...

@joshblack
Copy link
Contributor

@tw15egan I'm down with that, makes a lot of sense to me 👍

Copy link
Member

@laurenmrice laurenmrice 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 to me 👍🏻

I think that the Gray 10 theme may need a value change for the disabled-01 token, but I can look into that and open a separate issue.

@joshblack
Copy link
Contributor

bump @aledavila when you get a sec today 👀

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.

[Accordion] missing disabled state
4 participants