From b86aeeb8ff55262b75b7306229f6df9b4d3dcd01 Mon Sep 17 00:00:00 2001 From: Tobias Date: Mon, 3 Oct 2022 11:07:10 +0200 Subject: [PATCH] fix(Accordion): replace internal animation with HeightAnimation component --- .../uilib/components/accordion/properties.md | 1 + .../src/components/accordion/Accordion.js | 7 +- .../components/accordion/AccordionContent.js | 280 ++++++++---------- .../components/accordion/AccordionHeader.js | 4 + .../accordion/AccordionPropTypes.js | 2 + .../components/accordion/AccordionProvider.js | 1 + .../accordion/__tests__/Accordion.test.js | 103 +++++-- .../__snapshots__/Accordion.test.js.snap | 51 ++-- .../accordion/stories/Accordion.stories.js | 38 ++- .../accordion/style/_accordion.scss | 34 +-- 10 files changed, 272 insertions(+), 249 deletions(-) diff --git a/packages/dnb-design-system-portal/src/docs/uilib/components/accordion/properties.md b/packages/dnb-design-system-portal/src/docs/uilib/components/accordion/properties.md index b636d5310c4..b4016782977 100644 --- a/packages/dnb-design-system-portal/src/docs/uilib/components/accordion/properties.md +++ b/packages/dnb-design-system-portal/src/docs/uilib/components/accordion/properties.md @@ -29,6 +29,7 @@ These properties can send along with the `Accordion.Provider` or `Accordion.Grou | `heading_level` | _(optional)_ if `heading` is set to `true`, you can provide a numeric value to define a different heading level. Defaults to `2`. | | `disabled` | _(optional)_ if set to `true`, the accordion button will be disabled (dimmed). | | `skeleton` | _(optional)_ if set to `true`, an overlaying skeleton with animation will be shown. | +| `contentRef` | _(optional)_ send along a custom React Ref for `.dnb-accordion__content`. | | [Space](/uilib/components/space/properties) | _(optional)_ spacing properties like `top` or `bottom` are supported. | | Accordion.Provider and Accordion.Group Properties | Description | diff --git a/packages/dnb-eufemia/src/components/accordion/Accordion.js b/packages/dnb-eufemia/src/components/accordion/Accordion.js index 6b11f2a219b..ba9e1536a44 100644 --- a/packages/dnb-eufemia/src/components/accordion/Accordion.js +++ b/packages/dnb-eufemia/src/components/accordion/Accordion.js @@ -95,7 +95,6 @@ export default class Accordion extends React.PureComponent { ? isTrue(props.expanded) : isTrue(context?.expanded), group: props.group || context?.group, - no_animation: isTrue(props.no_animation || context?.no_animation), _listenForPropChanges: false, // make sure to not run DerivedState } @@ -240,7 +239,6 @@ export default class Accordion extends React.PureComponent { {(globalContext) => ( {(nestedContext) => { - const { no_animation } = this.state let { expanded } = this.state // use only the props from context, who are available here anyway @@ -271,7 +269,7 @@ export default class Accordion extends React.PureComponent { remember_state, disabled, skeleton, - no_animation: _no_animation, // eslint-disable-line + no_animation, expanded_ssr: _expanded_ssr, // eslint-disable-line children, @@ -289,6 +287,7 @@ export default class Accordion extends React.PureComponent { on_state_update, // eslint-disable-line custom_method, // eslint-disable-line custom_element, // eslint-disable-line + contentRef, // eslint-disable-line ...rest } = props @@ -300,7 +299,6 @@ export default class Accordion extends React.PureComponent { className: classnames( 'dnb-accordion', expanded && 'dnb-accordion--expanded', - no_animation && 'dnb-accordion--no-animation', variant && `dnb-accordion__variant--${variant}`, isTrue(prerender) && 'dnb-accordion--prerender', createSpacingClasses(props), @@ -336,6 +334,7 @@ export default class Accordion extends React.PureComponent { remember_state: isTrue(remember_state), disabled: isTrue(disabled), skeleton: isTrue(skeleton), + no_animation: isTrue(no_animation), callOnChange: this.callOnChangeHandler, } diff --git a/packages/dnb-eufemia/src/components/accordion/AccordionContent.js b/packages/dnb-eufemia/src/components/accordion/AccordionContent.js index 45961290050..687c302a6dc 100644 --- a/packages/dnb-eufemia/src/components/accordion/AccordionContent.js +++ b/packages/dnb-eufemia/src/components/accordion/AccordionContent.js @@ -5,131 +5,82 @@ import React from 'react' import PropTypes from 'prop-types' +import classnames from 'classnames' import { + warn, isTrue, validateDOMAttributes, processChildren, getPreviousSibling, } from '../../shared/component-helper' -import AnimateHeight from '../../shared/AnimateHeight' -import classnames from 'classnames' +import { useMediaQuery } from '../../shared' import AccordionContext from './AccordionContext' import { spacingPropTypes, createSpacingClasses, } from '../space/SpacingHelper' +import HeightAnimation from '../height-animation/HeightAnimation' -export default class AccordionContent extends React.PureComponent { - static contextType = AccordionContext - - static propTypes = { - instance: PropTypes.object, - ...spacingPropTypes, - className: PropTypes.string, - children: PropTypes.oneOfType([PropTypes.node, PropTypes.func]), - } - - static defaultProps = { - instance: null, - className: null, - children: null, - } - - static getContent(props) { - return processChildren(props) - } - - constructor(props, context) { - super(props) - this._ref = React.createRef() - - this.state = { - isInitial: !context.expanded, - isAnimating: false, - keepContentInDom: null, - } - - this.anim = new AnimateHeight() +export default function AccordionContent(props) { + const context = React.useContext(AccordionContext) - this.anim.onStart(() => { - this.setState({ - isAnimating: true, - }) - }) + const { + id, + expanded, + prerender, + prevent_rerender, + single_container, + disabled, + no_animation, + contentRef, + } = context - this.anim.onEnd(() => { - this.setState({ - isAnimating: false, - }) + const { className, children, instance, ...rest } = props - this.setState({ - keepContentInDom: this.context.expanded, - }) - }) + let elementRef = React.useRef(null) + const cacheRef = React.useRef(null) - if ( - props.instance && - Object.prototype.hasOwnProperty.call(props.instance, 'current') - ) { - props.instance.current = this - } + if (contentRef) { + elementRef = contentRef } - componentDidMount() { - this.anim.setElement( - this._ref.current, - getPreviousSibling( - 'dnb-accordion-group--single-container', - this._ref.current - ) - ) - } + const setContainerHeight = () => { + const { single_container } = context - componentWillUnmount() { - this.anim.remove() - } + if (single_container) { + const contentElem = elementRef.current + if (contentElem) { + try { + contentElem.style.height = '' - componentDidUpdate(prevProps) { - const { expanded, single_container } = this.context - if (expanded !== this.state._expanded) { - const isInitial = !expanded && this.state.isInitial - this.setState( - { - _expanded: expanded, - isInitial: false, - keepContentInDom: true, - }, - () => { - if (expanded) { - this.anim.open({ animate: !isInitial }) - } else { - this.anim.close({ animate: !isInitial }) + const containerElement = getPreviousSibling( + 'dnb-accordion-group--single-container', + contentElem + ) + + if (no_animation) { + containerElement.style.transitionDuration = '1ms' } - } - ) - } - if ( - isTrue(single_container) && - AccordionContent.getContent(prevProps) !== - AccordionContent.getContent(this.props) - ) { - this.anim.setContainerHeight() + const minHeight = + (contentElem.offsetHeight + contentElem.offsetTop) / 16 + containerElement.style.minHeight = `${minHeight}rem` + } catch (e) { + warn(e) + } + } } } - setContainerHeight() { - this.anim?.setContainerHeight() - } + const renderContent = () => { + const children = processChildren(props) - renderContent() { - const children = AccordionContent.getContent(this.props) const { expanded, prerender, prevent_rerender, prevent_rerender_conditional, - } = this.context + } = context let content = children @@ -137,85 +88,108 @@ export default class AccordionContent extends React.PureComponent { content =

{content}

} - content = - expanded || - prerender || - this.state.keepContentInDom || - this.state.isAnimating - ? children - : null - if (isTrue(prevent_rerender)) { + /** + * Ensure we do not render, if it is not expanded + */ + if (!(expanded || prerender)) { + content = null + } + // update the cache if children is not the same anymore if ( isTrue(prevent_rerender_conditional) && - this._cache !== content + cacheRef.current !== content ) { - this._cache = content + cacheRef.current = content } - if (this._cache) { - content = this._cache + if (cacheRef.current) { + content = cacheRef.current } else { - this._cache = content + cacheRef.current = content } } return content } - render() { - const { - className, - instance, // eslint-disable-line - ...rest - } = this.props - const { keepContentInDom, isAnimating } = this.state - - const { id, expanded, disabled } = this.context - - const content = this.renderContent() - - const wrapperParams = { - className: classnames( - 'dnb-accordion__content', - !expanded && !keepContentInDom && 'dnb-accordion__content--hidden', - isAnimating && 'dnb-accordion__content--is-animating', - className - ), - ...rest, + React.useEffect(() => { + if (expanded && isTrue(single_container)) { + setContainerHeight() } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [children, expanded, single_container]) - const innerParams = { - id: `${id}-content`, - role: 'region', - 'aria-labelledby': `${id}-header`, - className: classnames( - 'dnb-accordion__content__inner', - !expanded && - !keepContentInDom && - 'dnb-accordion__content__inner--remove-content', - createSpacingClasses(rest) - ), + React.useState(() => { + if ( + instance && + Object.prototype.hasOwnProperty.call(instance, 'current') + ) { + instance.current = { setContainerHeight } } + }) - if (expanded) { - innerParams['aria-expanded'] = true - } + const isSmallScreen = useMediaQuery({ + when: { max: 'small' }, + }) - if (!expanded || disabled) { - innerParams.disabled = true - innerParams['aria-hidden'] = true - } + const content = renderContent() - // to remove spacing props - validateDOMAttributes(this.props, wrapperParams) - validateDOMAttributes(null, innerParams) + const wrapperParams = { + className: classnames('dnb-accordion__content', className), + ...rest, + } + + const keepInDOM = prerender || prevent_rerender + + const innerParams = { + id: `${id}-content`, + 'aria-labelledby': `${id}-header`, + className: classnames( + 'dnb-accordion__content__inner', + createSpacingClasses(rest) + ), + } + + if (expanded) { + innerParams['aria-expanded'] = true + } - return ( -
-
{content}
-
- ) + if (!expanded || disabled) { + innerParams.disabled = true + innerParams['aria-hidden'] = true } + + // to remove spacing props + validateDOMAttributes(props, wrapperParams) + validateDOMAttributes(null, innerParams) + + const animate = + !no_animation && (single_container ? isSmallScreen : true) + + return ( + +
{content}
+ + ) +} + +AccordionContent.propTypes = { + instance: PropTypes.object, + ...spacingPropTypes, + className: PropTypes.string, + children: PropTypes.oneOfType([PropTypes.node, PropTypes.func]), +} + +AccordionContent.defaultProps = { + instance: null, + className: null, + children: null, } diff --git a/packages/dnb-eufemia/src/components/accordion/AccordionHeader.js b/packages/dnb-eufemia/src/components/accordion/AccordionHeader.js index 7d53eb74f48..6c1b8485f02 100644 --- a/packages/dnb-eufemia/src/components/accordion/AccordionHeader.js +++ b/packages/dnb-eufemia/src/components/accordion/AccordionHeader.js @@ -162,6 +162,7 @@ export default class AccordionHeader extends React.PureComponent { icon_size: PropTypes.string, disabled: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]), skeleton: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]), + no_animation: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]), ...spacingPropTypes, @@ -186,6 +187,7 @@ export default class AccordionHeader extends React.PureComponent { icon_size: 'medium', disabled: null, skeleton: null, + no_animation: null, className: null, children: null, @@ -264,6 +266,7 @@ export default class AccordionHeader extends React.PureComponent { icon_size, disabled, skeleton, + no_animation, } = props const { @@ -382,6 +385,7 @@ export default class AccordionHeader extends React.PureComponent { hover && hadClick && 'dnb-accordion--hover', !this.canClick() && 'dnb-accordion__header--prevent-click', description && 'dnb-accordion__header--description', + no_animation && 'dnb-accordion__header--no-animation', createSkeletonClass('font', skeleton, this.context), createSpacingClasses(rest), className diff --git a/packages/dnb-eufemia/src/components/accordion/AccordionPropTypes.js b/packages/dnb-eufemia/src/components/accordion/AccordionPropTypes.js index 9bf96995f8b..27c688b9d80 100644 --- a/packages/dnb-eufemia/src/components/accordion/AccordionPropTypes.js +++ b/packages/dnb-eufemia/src/components/accordion/AccordionPropTypes.js @@ -21,6 +21,7 @@ export const accordionPropTypes = { PropTypes.bool, ]), remember_state: PropTypes.oneOfType([PropTypes.string, PropTypes.bool]), + contentRef: PropTypes.object, flush_remembered_state: PropTypes.oneOfType([ PropTypes.string, PropTypes.bool, @@ -72,6 +73,7 @@ export const accordionDefaultProps = { prevent_rerender: null, prevent_rerender_conditional: null, remember_state: null, + contentRef: null, flush_remembered_state: null, single_container: null, variant: 'outlined', diff --git a/packages/dnb-eufemia/src/components/accordion/AccordionProvider.js b/packages/dnb-eufemia/src/components/accordion/AccordionProvider.js index 3e60fccb884..d3a749d0d00 100644 --- a/packages/dnb-eufemia/src/components/accordion/AccordionProvider.js +++ b/packages/dnb-eufemia/src/components/accordion/AccordionProvider.js @@ -82,6 +82,7 @@ export default class AccordionGroup extends React.PureComponent { prerender, // eslint-disable-line prevent_rerender, // eslint-disable-line single_container, // eslint-disable-line + contentRef, // eslint-disable-line allow_close_all, // eslint-disable-line remember_state, // eslint-disable-line flush_remembered_state, // eslint-disable-line diff --git a/packages/dnb-eufemia/src/components/accordion/__tests__/Accordion.test.js b/packages/dnb-eufemia/src/components/accordion/__tests__/Accordion.test.js index c7df7450f98..8de0f8b553f 100644 --- a/packages/dnb-eufemia/src/components/accordion/__tests__/Accordion.test.js +++ b/packages/dnb-eufemia/src/components/accordion/__tests__/Accordion.test.js @@ -16,6 +16,9 @@ import { subtract_medium as SubtractIcon, } from '../../../icons' import { render, fireEvent } from '@testing-library/react' +import MatchMediaMock from 'jest-matchmedia-mock' + +new MatchMediaMock() const props = {} props.id = 'accordion' @@ -78,6 +81,29 @@ describe('Accordion component', () => { expect(my_event.mock.calls[1][0].expanded).toBe(false) }) + it('uses a p element when string content is given', () => { + const { rerender } = render( + + string content + + ) + + expect(document.querySelector('.dnb-p').textContent).toBe( + 'string content' + ) + + rerender( + + no string content + + ) + + expect(document.querySelector('.dnb-p')).toBeFalsy() + expect(document.querySelector('.no-string').textContent).toBe( + 'no string content' + ) + }) + it('has a disabled attribute, once we set disabled to true', () => { const { rerender } = render() @@ -89,6 +115,18 @@ describe('Accordion component', () => { ).toBe(true) }) + it('has correct classes when no_animation', () => { + render() + + expect( + Array.from( + document.querySelector('.dnb-accordion__header').classList + ) + ).toEqual( + expect.arrayContaining(['dnb-accordion__header--no-animation']) + ) + }) + it('supports an icon for expanded state ', () => { render( { render() expect( - Array.from(document.querySelector('.dnb-accordion__content')) - ).toBeTruthy() - - expect( - document - .querySelector('.dnb-accordion__content__inner') - .classList.contains( - 'dnb-accordion__content__inner--remove-content' - ) - ).toBe(true) + Array.from( + document.querySelector('.dnb-accordion__content').classList + ) + ).toEqual(expect.arrayContaining(['dnb-height-animation--hidden'])) fireEvent.click(document.querySelector('.dnb-accordion__header')) - expect(document.querySelector('.dnb-accordion__content')).toBeTruthy() expect( Array.from( document.querySelector('.dnb-accordion__content').classList ) - ).toEqual([ - 'dnb-accordion__content', - 'dnb-accordion__content--is-animating', - ]) - - expect( - document - .querySelector('.dnb-accordion__content__inner') - .classList.contains( - 'dnb-accordion__content__inner--remove-content' - ) - ).toBe(false) + ).toEqual(expect.arrayContaining(['dnb-height-animation--is-in-dom'])) }) it('should validate with ARIA rules', async () => { @@ -226,10 +246,8 @@ describe('Accordion group component', () => { document.querySelector('#accordion-1 .dnb-accordion__content') ).toBeTruthy() expect( - document.querySelector( - '#accordion-2 .dnb-accordion__content--hidden' - ) - ).toBeTruthy() + document.querySelector('#accordion-2 .dnb-accordion__content') + ).toBeFalsy() }) it('has "on_change" event which will trigger on a button click', () => { @@ -301,8 +319,8 @@ describe('Accordion container component', () => { ) } - it('has only to render the expanded accordion content', () => { - render( + const Container = (props) => { + return ( <> { single_container prevent_rerender remember_state + no_animation + {...props} > Accordion 1 @@ -326,6 +346,10 @@ describe('Accordion container component', () => { ) + } + + it('has only to render the expanded accordion content', () => { + render() expect(document.querySelector('button#increment').textContent).toBe( '1' @@ -388,6 +412,27 @@ describe('Accordion container component', () => { 'true' ) }) + + it('will set minHeight', async () => { + const contentRef = React.createRef() + + render() + + const contentElem = contentRef.current + + jest.spyOn(contentElem, 'offsetHeight', 'get').mockReturnValue(48) + jest.spyOn(contentElem, 'offsetTop', 'get').mockReturnValue(48) + + fireEvent.click( + document.querySelector('#accordion-1 .dnb-accordion__header') + ) + + expect( + document + .querySelector('.dnb-accordion-group--single-container') + .getAttribute('style') + ).toBe('transition-duration: 1ms; min-height: 6rem;') + }) }) describe('Accordion scss', () => { diff --git a/packages/dnb-eufemia/src/components/accordion/__tests__/__snapshots__/Accordion.test.js.snap b/packages/dnb-eufemia/src/components/accordion/__tests__/__snapshots__/Accordion.test.js.snap index 9fb99be5b15..611a2b4023a 100644 --- a/packages/dnb-eufemia/src/components/accordion/__tests__/__snapshots__/Accordion.test.js.snap +++ b/packages/dnb-eufemia/src/components/accordion/__tests__/__snapshots__/Accordion.test.js.snap @@ -6,6 +6,7 @@ exports[`Accordion component have to match snapshot 1`] = ` attributes={null} class={null} className={null} + contentRef={null} custom_element={null} custom_method={null} disabled={null} @@ -34,7 +35,7 @@ exports[`Accordion component have to match snapshot 1`] = ` variant="default" >
-
-
-
+
@@ -544,29 +544,21 @@ exports[`Accordion scss have to match snapshot 1`] = ` margin-top: 0.25rem; } .dnb-accordion__header__description + .dnb-accordion__header__title { margin-top: 0.25rem; } - .dnb-accordion--no-animation .dnb-accordion__header__icon, - html[data-visual-test] .dnb-accordion__header__icon { - transition-duration: 1ms !important; } .dnb-accordion--expanded > .dnb-accordion__header .dnb-accordion__header__icon { transform: rotate(-180deg); } + .dnb-accordion > .dnb-accordion__header--no-animation .dnb-accordion__header__icon, + html[data-visual-test] .dnb-accordion > .dnb-accordion__header--no-animation .dnb-accordion__header__icon { + transition-duration: 1ms !important; } .dnb-accordion__content { display: flex; - opacity: 1; - visibility: visible; width: 100%; will-change: height; transition: height 400ms var(--accordion-easing), opacity 600ms var(--accordion-easing); } - .dnb-accordion__content--hidden { - visibility: hidden; - height: 0; - opacity: 0; } - .dnb-accordion__content--is-animating { - overflow: hidden; } .dnb-accordion__content__inner { width: 100%; margin-top: 1rem; } - .dnb-accordion__content__inner--remove-content { - display: none; } + .dnb-accordion__content.dnb-height-animation--hidden .dnb-accordion__content__inner { + display: none; } .dnb-accordion-group--single-container { transition: min-height 1s var(--accordion-easing); } .dnb-accordion-group--single-container .dnb-accordion { @@ -583,9 +575,6 @@ exports[`Accordion scss have to match snapshot 1`] = ` @media screen and (min-width: 40em) { .dnb-accordion-group--single-container .dnb-accordion > .dnb-accordion__header .dnb-accordion__header__icon { transform: rotate(-90deg); } } - .dnb-accordion--no-animation .dnb-accordion__content, - html[data-visual-test] .dnb-accordion__content { - transition-duration: 1ms !important; } .dnb-accordion-group--single-container .dnb-accordion-group__children { max-width: 60rem; } @media screen and (min-width: 40em) { diff --git a/packages/dnb-eufemia/src/components/accordion/stories/Accordion.stories.js b/packages/dnb-eufemia/src/components/accordion/stories/Accordion.stories.js index 8f5315af1db..96567a5b623 100644 --- a/packages/dnb-eufemia/src/components/accordion/stories/Accordion.stories.js +++ b/packages/dnb-eufemia/src/components/accordion/stories/Accordion.stories.js @@ -202,10 +202,8 @@ function AccordionWithContainer() { const [changeHeight] = React.useState(() => ({ ref1, ref2, ref3 })) const [flushCache, flushCacheNow] = React.useState(false) const [count, setCount] = React.useState(1) - // console.log('flushCache', flushCache) return ( <> - {count} @@ -266,6 +264,15 @@ function AccordionWithContainer() {

+
+ + ) } + function ChangingContent({ changeHeight, children }) { const [contentSize, changeContentSize] = React.useState(false) React.useLayoutEffect(() => { diff --git a/packages/dnb-eufemia/src/components/accordion/style/_accordion.scss b/packages/dnb-eufemia/src/components/accordion/style/_accordion.scss index 6118a443046..42a761a2dc4 100644 --- a/packages/dnb-eufemia/src/components/accordion/style/_accordion.scss +++ b/packages/dnb-eufemia/src/components/accordion/style/_accordion.scss @@ -114,44 +114,30 @@ } } - &--no-animation &__header, - html[data-visual-test] &__header { - &__icon { - transition-duration: 1ms !important; - } - } - &--expanded > &__header &__header__icon { transform: rotate(-180deg); } + & > &__header--no-animation &__header__icon, + html[data-visual-test] & > &__header--no-animation &__header__icon { + transition-duration: 1ms !important; + } &__content { display: flex; - opacity: 1; - visibility: visible; width: 100%; will-change: height; transition: height 400ms var(--accordion-easing), opacity 600ms var(--accordion-easing); - &--hidden { - visibility: hidden; - height: 0; - opacity: 0; - } - - &--is-animating { - overflow: hidden; // Is needed in order to have a smooth animation, hiding the content nicely. - } - &__inner { width: 100%; margin-top: 1rem; + } - &--remove-content { - display: none; - } + // When "prerender" prop is true + &.dnb-height-animation--hidden &__inner { + display: none; } } @@ -193,10 +179,6 @@ transform: rotate(-90deg); } } - &--no-animation &__content, - html[data-visual-test] &__content { - transition-duration: 1ms !important; - } &-group { &--single-container & {