From a792365865aa693474ee3300d39524b12b9e6a64 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 | 5 +- .../components/accordion/AccordionContent.js | 280 ++++++++---------- .../accordion/AccordionPropTypes.js | 2 + .../components/accordion/AccordionProvider.js | 1 + .../accordion/__tests__/Accordion.test.js | 91 ++++-- .../__snapshots__/Accordion.test.js.snap | 39 +-- .../accordion/stories/Accordion.stories.js | 38 ++- .../accordion/style/_accordion.scss | 19 +- 9 files changed, 246 insertions(+), 230 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..b69cef16e39 100644 --- a/packages/dnb-eufemia/src/components/accordion/Accordion.js +++ b/packages/dnb-eufemia/src/components/accordion/Accordion.js @@ -240,7 +240,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 +270,7 @@ export default class Accordion extends React.PureComponent { remember_state, disabled, skeleton, - no_animation: _no_animation, // eslint-disable-line + no_animation, // eslint-disable-line expanded_ssr: _expanded_ssr, // eslint-disable-line children, @@ -289,6 +288,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 +300,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), 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/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..f41fc9a8d61 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() @@ -136,36 +162,18 @@ describe('Accordion component', () => { 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 +234,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 +307,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 +334,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 +400,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..be19b456d30 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 @@ -20,6 +20,7 @@ exports[`Accordion component have to match snapshot 1`] = ` icon_position={null} icon_size="medium" id="accordion" + innerRef={null} left_component={null} no_animation={true} on_change={null} @@ -34,7 +35,7 @@ exports[`Accordion component have to match snapshot 1`] = ` variant="default" >
-
-
-
+
@@ -551,22 +550,14 @@ exports[`Accordion scss have to match snapshot 1`] = ` transform: rotate(-180deg); } .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 { 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..edbbb87922e 100644 --- a/packages/dnb-eufemia/src/components/accordion/style/_accordion.scss +++ b/packages/dnb-eufemia/src/components/accordion/style/_accordion.scss @@ -127,31 +127,20 @@ &__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; } }