Skip to content

Add Borderless variant to Accordion #2446

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

Merged
merged 2 commits into from
Jan 17, 2025
Merged

Conversation

benlister-okta
Copy link
Contributor

DES-6886

Summary

  • Adds borderless variant to accordion to match Figma
  • Updates accordion storybook docs with correct prop type for label (string not ReactNode)

Testing & Screenshots

CleanShot 2024-12-19 at 14 27 32@2x

  • I have confirmed this change with my designer and the Odyssey Design Team.

@benlister-okta benlister-okta requested a review from a team as a code owner December 19, 2024 22:33
@@ -82,6 +82,9 @@ export const components = ({
borderRadius: 0,
boxShadow: "none",

'&[data-ods-type="accordion"][data-ods-variant="borderless"]': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably don't need the extra selector here

Suggested change
'&[data-ods-type="accordion"][data-ods-variant="borderless"]': {
'&[data-ods-variant="borderless"]': {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, initially I thought having type="accordion and variant="borderless" would be helpful for targeting but since we don't have global CSS there shouldn't be confusion, plus it's one less attribute to pass.

@@ -81,6 +88,8 @@ const Accordion = ({
return (
<MuiAccordion
data-se={testId}
data-ods-type="accordion"
data-ods-variant={variant}
defaultExpanded={isDefaultExpanded}
disabled={isDisabled}
disableGutters
Copy link
Contributor

Choose a reason for hiding this comment

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

@benlister-okta In the borderless version, do we want the gutters? So, if we have 2 of these next to each other, they give each other space?

Copy link
Contributor

Choose a reason for hiding this comment

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

That seemingly only applies when they are open though... wonder if we want to add some margin if these are next to each other. Any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CleanShot 2025-01-06 at 14 56 00@2x
In Figma we have them without a gutter and I think it could work in code the same way. It may depend on the context but since the content is passed in via ReactNode, the consumer could also control this technically.

@@ -82,6 +82,14 @@ export const components = ({
borderRadius: 0,
boxShadow: "none",

"&.MuiPaper-root.MuiAccordion-root[data-ods-variant='borderless']": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this not work with just &[data-ods-variant='borderless']? Feels like it should

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to need the specificity for some reason. Previously I had the 2 data attributes and that worked, but for some reason, it gets overridden without the parent class attached 🤷🏼‍♂️

@oktapp-aperture-okta oktapp-aperture-okta bot merged commit 6c7458c into main Jan 17, 2025
1 check passed
@oktapp-aperture-okta oktapp-aperture-okta bot deleted the bl_accordionfix branch January 17, 2025 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants