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 align prop to component and skeleton to change heading alignment #4754

Merged
merged 12 commits into from
Dec 5, 2019
Merged

feat(accordion): add align prop to component and skeleton to change heading alignment #4754

merged 12 commits into from
Dec 5, 2019

Conversation

jendowns
Copy link
Contributor

Closes #4584

Proposal to add an align prop to the Accordion and AccordionSkeleton.

By default, align="end", which is the current Carbon default.
If align="start", then the heading text + arrow order is reversed, with the arrow being at the "start" of the heading row.

Changelog

New

  • add align prop with class name and styles

Changed

  • update storybook to include align prop knobs
  • update snapshots

Testing / Reviewing

In the carbon-components-react deployment for this PR, open the knobs & use the align knob to change alignment of the Accordion and AccordionSkeleton headings.

@jendowns jendowns requested a review from a team as a code owner November 22, 2019 17:04
@ghost ghost requested review from aledavila and joshblack November 22, 2019 17:04
@jendowns jendowns changed the title 4584 accordion alignment feat(accordion): add align prop to component and skeleton to change heading alignment Nov 22, 2019
@@ -23,6 +23,22 @@
width: 100%;
}

.#{$prefix}--accordion--start {
.#{$prefix}--accordion__heading {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you un-nest selectors here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @joshblack! Just pushed an update that unnests these selectors 👍

@netlify
Copy link

netlify bot commented Nov 22, 2019

Deploy preview for carbon-components-react ready!

Built with commit bf7fd67

https://deploy-preview-4754--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Nov 22, 2019

Deploy preview for the-carbon-components ready!

Built with commit bf7fd67

https://deploy-preview-4754--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Nov 22, 2019

Deploy preview for carbon-elements ready!

Built with commit bf7fd67

https://deploy-preview-4754--carbon-elements.netlify.com

@asudoh asudoh requested a review from a team November 23, 2019 01:38
@ghost ghost requested review from jillianhowarth and removed request for a team November 23, 2019 01:38
@joshblack
Copy link
Contributor

@carbon-design-system/design was curious if this was functionality we wanted to add in 👀

@aagonzales
Copy link
Member

aagonzales commented Dec 2, 2019

I think if it's an easy to add and switch between the two then its ok with design. We'll need to maybe think of some guidance on how the two should be used and if there's a usage difference or if its just an aesthetic chose a product can make and then consistently use.

@jendowns
Copy link
Contributor Author

jendowns commented Dec 2, 2019

Thank you @aagonzales! As an example, we have a component in the IBM Security portoflio called a FilterPanel that uses inner Accordions with "start" alignment applied: https://ibm-security.carbondesignsystem.com/?path=/story/patterns-filterpanel--default

Screen Shot 2019-12-02 at 1 48 18 PM

I'm sure @cameroncalder has more examples / justifications he can share in Slack though! There are probably other patterns out there as well, aside from the component above.

Copy link
Contributor

@aledavila aledavila left a comment

Choose a reason for hiding this comment

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

lgtm

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] add align prop to align chevron to "start" of heading
6 participants