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.

Sign up for GitHub

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

Jump to bottom

feat(Accordion): Added support for bordered, display large … #6085

Merged
merged 2 commits into from Jul 30, 2021
Merged

feat(Accordion): Added support for bordered, display large … #6085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2629422
feat(Accordion): Added support for bordered modifiers, display large …
Jul 29, 2021
e38fa4b
update prop name to be flexible
Jul 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 16 additions & 1 deletion packages/react-core/src/components/Accordion/Accordion.tsx
View file
Open in desktop
Expand Up @@ -14,6 +14,10 @@ export interface AccordionProps extends React.HTMLProps<HTMLDListElement> {
headingLevel?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6';
/** Flag to indicate whether use definition list or div */
asDefinitionList?: boolean;
/** Flag to indicate the accordion had a border */
isBordered?: boolean;
/** Display size variant. */
displaySize?: 'default' | 'large';
}

export const Accordion: React.FunctionComponent<AccordionProps> = ({
Expand All @@ -22,11 +26,22 @@ export const Accordion: React.FunctionComponent<AccordionProps> = ({
'aria-label': ariaLabel = '',
headingLevel = 'h3',
asDefinitionList = true,
isBordered = false,
displaySize = 'default',
...props
}: AccordionProps) => {
const AccordionList: any = asDefinitionList ? 'dl' : 'div';
return (
<AccordionList className={css(styles.accordion, className)} aria-label={ariaLabel} {...props}>
<AccordionList
className={css(
styles.accordion,
isBordered && styles.modifiers.bordered,
displaySize === 'large' && styles.modifiers.displayLg,
className
)}
aria-label={ariaLabel}
{...props}
>
<AccordionContext.Provider
value={{
ContentContainer: asDefinitionList ? 'dd' : 'div',
Expand Down
6 changes: 5 additions & 1 deletion packages/react-core/src/components/Accordion/AccordionContent.tsx
View file
Open in desktop
Expand Up @@ -2,6 +2,7 @@ import * as React from 'react';
import { css } from '@patternfly/react-styles';
import styles from '@patternfly/react-styles/css/components/Accordion/accordion';
import { AccordionContext } from './AccordionContext';
import { AccordionExpandedContentBody } from './AccordionExpandedContentBody';

export interface AccordionContentProps extends React.HTMLProps<HTMLDivElement> {
/** Content rendered inside the Accordion */
Expand All @@ -18,6 +19,8 @@ export interface AccordionContentProps extends React.HTMLProps<HTMLDivElement> {
'aria-label'?: string;
/** Component to use as content container */
component?: React.ElementType;
/** Flag indicating content is custom. Expanded content Body wrapper will be removed from children. This allows multiple bodies to be rendered as content. */
isCustomContent?: React.ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an interesting way to handle this. I think we don't usually conditionally expect the consumer to wrap elements in a body component. I wonder if it's be more consistent if we allowed people to just pass a single child, or an array of children? and we wrap things in body components for them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicole I was debating both approaches. I initially stared with the single Node or array. I was thinking in the long run it would be nice to handle this similar to other components with bodies (Card, Drawer) where the consumer composes it themself, that is why I took this approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better then, to always tell the consumer to wrap content in a body?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could update to always wrapping the content in a body in the next breaking release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolethoen that would break consumers who are already using the component.
@kmcfaul that is what I was thinking long term.

I am ok with either approach here... Like I said, I was debating which one to go with.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think either way will work, as long as it's clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kmcfaul what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think having the custom prop is good until we can refactor 👍

}

export const AccordionContent: React.FunctionComponent<AccordionContentProps> = ({
Expand All @@ -26,6 +29,7 @@ export const AccordionContent: React.FunctionComponent<AccordionContentProps> =
id = '',
isHidden = false,
isFixed = false,
isCustomContent = false,
'aria-label': ariaLabel = '',
component,
...props
Expand All @@ -46,7 +50,7 @@ export const AccordionContent: React.FunctionComponent<AccordionContentProps> =
aria-label={ariaLabel}
{...props}
>
<div className={css(styles.accordionExpandedContentBody)}>{children}</div>
{isCustomContent ? children : <AccordionExpandedContentBody>{children}</AccordionExpandedContentBody>}
</Container>
);
}}
Expand Down
13 changes: 13 additions & 0 deletions packages/react-core/src/components/Accordion/AccordionExpandedContentBody.tsx
View file
Open in desktop
@@ -0,0 +1,13 @@
import * as React from 'react';
import { css } from '@patternfly/react-styles';
import styles from '@patternfly/react-styles/css/components/Accordion/accordion';

export interface AccordionExpandedContentBodyProps {
/** Content rendered inside the accordion content body */
children?: React.ReactNode;
}

export const AccordionExpandedContentBody: React.FunctionComponent<AccordionExpandedContentBodyProps> = ({
children = null
}: AccordionExpandedContentBodyProps) => <div className={css(styles.accordionExpandedContentBody)}>{children}</div>;
AccordionExpandedContentBody.displayName = 'AccordionExpandedContentBody';
40 changes: 40 additions & 0 deletions packages/react-core/src/components/Accordion/__tests__/Accordion.test.tsx
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ import { Accordion } from '../Accordion';
import { AccordionToggle } from '../AccordionToggle';
import { AccordionContent } from '../AccordionContent';
import { AccordionItem } from '../AccordionItem';
import { AccordionExpandedContentBody } from '../AccordionExpandedContentBody';

describe('Accordion', () => {
test('Accordion default', () => {
Expand Down Expand Up @@ -61,4 +62,43 @@ describe('Accordion', () => {
expect(view.find(AccordionToggle).getDOMNode().tagName).toBe(container.toLocaleUpperCase());
expect(view.find(AccordionContent).getDOMNode().tagName).toBe(container.toLocaleUpperCase());
});

test('Accordion bordered', () => {
const view = shallow(
<Accordion isBordered>
<AccordionItem>
<AccordionToggle id="item-1">Item One</AccordionToggle>
<AccordionContent>Item One Content</AccordionContent>
</AccordionItem>
</Accordion>
);
expect(view.render()).toMatchSnapshot();
});

test('Accordion display large', () => {
const view = shallow(
<Accordion displaySize='large'>
<AccordionItem>
<AccordionToggle id="item-1">Item One</AccordionToggle>
<AccordionContent>Item One Content</AccordionContent>
</AccordionItem>
</Accordion>
);
expect(view.render()).toMatchSnapshot();
});

test('Accordion custom content', () => {
const view = shallow(
<Accordion>
<AccordionItem>
<AccordionToggle id="item-1">Item One</AccordionToggle>
<AccordionContent isCustomContent>
<AccordionExpandedContentBody>Item one content body 1</AccordionExpandedContentBody>
<AccordionExpandedContentBody>Item one Content body 2</AccordionExpandedContentBody>
</AccordionContent>
</AccordionItem>
</Accordion>
);
expect(view.render()).toMatchSnapshot();
});
});
155 changes: 155 additions & 0 deletions packages/react-core/src/components/Accordion/__tests__/__snapshots__/Accordion.test.tsx.snap
View file
Open in desktop
@@ -1,12 +1,167 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`Accordion Accordion bordered 1`] = `
<dl
aria-label=""
class="pf-c-accordion pf-m-bordered"
>
<dt>
<button
aria-expanded="false"
class="pf-c-accordion__toggle"
id="item-1"
type="button"
>
<span
class="pf-c-accordion__toggle-text"
>
Item One
</span>
<span
class="pf-c-accordion__toggle-icon"
>
<svg
aria-hidden="true"
fill="currentColor"
height="1em"
role="img"
style="vertical-align:-0.125em"
viewBox="0 0 256 512"
width="1em"
>
<path
d="M224.3 273l-136 136c-9.4 9.4-24.6 9.4-33.9 0l-22.6-22.6c-9.4-9.4-9.4-24.6 0-33.9l96.4-96.4-96.4-96.4c-9.4-9.4-9.4-24.6 0-33.9L54.3 103c9.4-9.4 24.6-9.4 33.9 0l136 136c9.5 9.4 9.5 24.6.1 34z"
/>
</svg>
</span>
</button>
</dt>
<dd
aria-label=""
class="pf-c-accordion__expanded-content pf-m-expanded"
id=""
>
<div
class="pf-c-accordion__expanded-content-body"
>
Item One Content
</div>
</dd>
</dl>
`;

exports[`Accordion Accordion custom content 1`] = `
<dl
aria-label=""
class="pf-c-accordion"
>
<dt>
<button
aria-expanded="false"
class="pf-c-accordion__toggle"
id="item-1"
type="button"
>
<span
class="pf-c-accordion__toggle-text"
>
Item One
</span>
<span
class="pf-c-accordion__toggle-icon"
>
<svg
aria-hidden="true"
fill="currentColor"
height="1em"
role="img"
style="vertical-align:-0.125em"
viewBox="0 0 256 512"
width="1em"
>
<path
d="M224.3 273l-136 136c-9.4 9.4-24.6 9.4-33.9 0l-22.6-22.6c-9.4-9.4-9.4-24.6 0-33.9l96.4-96.4-96.4-96.4c-9.4-9.4-9.4-24.6 0-33.9L54.3 103c9.4-9.4 24.6-9.4 33.9 0l136 136c9.5 9.4 9.5 24.6.1 34z"
/>
</svg>
</span>
</button>
</dt>
<dd
aria-label=""
class="pf-c-accordion__expanded-content pf-m-expanded"
id=""
>
<div
class="pf-c-accordion__expanded-content-body"
>
Item one content body 1
</div>
<div
class="pf-c-accordion__expanded-content-body"
>
Item one Content body 2
</div>
</dd>
</dl>
`;

exports[`Accordion Accordion default 1`] = `
<dl
aria-label="this is a simple accordion"
class="pf-c-accordion"
/>
`;

exports[`Accordion Accordion display large 1`] = `
<dl
aria-label=""
class="pf-c-accordion pf-m-display-lg"
>
<dt>
<button
aria-expanded="false"
class="pf-c-accordion__toggle"
id="item-1"
type="button"
>
<span
class="pf-c-accordion__toggle-text"
>
Item One
</span>
<span
class="pf-c-accordion__toggle-icon"
>
<svg
aria-hidden="true"
fill="currentColor"
height="1em"
role="img"
style="vertical-align:-0.125em"
viewBox="0 0 256 512"
width="1em"
>
<path
d="M224.3 273l-136 136c-9.4 9.4-24.6 9.4-33.9 0l-22.6-22.6c-9.4-9.4-9.4-24.6 0-33.9l96.4-96.4-96.4-96.4c-9.4-9.4-9.4-24.6 0-33.9L54.3 103c9.4-9.4 24.6-9.4 33.9 0l136 136c9.5 9.4 9.5 24.6.1 34z"
/>
</svg>
</span>
</button>
</dt>
<dd
aria-label=""
class="pf-c-accordion__expanded-content pf-m-expanded"
id=""
>
<div
class="pf-c-accordion__expanded-content-body"
>
Item One Content
</div>
</dd>
</dl>
`;

exports[`Accordion Accordion with non-default headingLevel 1`] = `
<div
aria-label=""
Expand Down