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

fix(Offcanvas): fix children mounting 2x on show when not responsive #6416

Merged
merged 1 commit into from Aug 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion src/NavbarOffcanvas.tsx
Expand Up @@ -9,7 +9,14 @@ const NavbarOffcanvas = React.forwardRef<HTMLDivElement, NavbarOffcanvasProps>(
(props, ref) => {
const context = useContext(NavbarContext);

return <Offcanvas ref={ref} show={!!context?.expanded} {...props} />;
return (
<Offcanvas
ref={ref}
show={!!context?.expanded}
{...props}
renderStaticNode
/>
);
},
);

Expand Down
19 changes: 17 additions & 2 deletions src/Offcanvas.tsx
Expand Up @@ -45,6 +45,7 @@ export interface OffcanvasProps
scroll?: boolean;
placement?: OffcanvasPlacement;
responsive?: 'sm' | 'md' | 'lg' | 'xl' | 'xxl' | string;
renderStaticNode?: boolean;
}

const propTypes = {
Expand Down Expand Up @@ -176,6 +177,13 @@ const propTypes = {
*/
container: PropTypes.any,

/**
* For internal use to render static node from NavbarOffcanvas.
*
* @private
*/
renderStaticNode: PropTypes.bool,

'aria-labelledby': PropTypes.string,
};

Expand All @@ -188,6 +196,7 @@ const defaultProps: Partial<OffcanvasProps> = {
enforceFocus: true,
restoreFocus: true,
placement: 'start',
renderStaticNode: false,
};

function DialogTransition(props) {
Expand Down Expand Up @@ -231,6 +240,7 @@ const Offcanvas: BsPrefixRefForwardingComponent<'div', OffcanvasProps> =
onExited,
backdropClassName,
manager: propsManager,
renderStaticNode,
...props
},
ref,
Expand Down Expand Up @@ -317,9 +327,14 @@ const Offcanvas: BsPrefixRefForwardingComponent<'div', OffcanvasProps> =
<>
{/*
Only render static elements when offcanvas isn't shown so we
don't duplicate elements
don't duplicate elements.

TODO: Should follow bootstrap behavior and don't unmount children
when show={false} in BaseModal. Will do this next major version.
*/}
{!showOffcanvas && renderDialog({})}
{!showOffcanvas &&
(responsive || renderStaticNode) &&
renderDialog({})}

<ModalContext.Provider value={modalContext}>
<BaseModal
Expand Down
29 changes: 29 additions & 0 deletions test/OffcanvasSpec.tsx
@@ -1,4 +1,5 @@
import * as React from 'react';
import { useEffect } from 'react';
import { expect } from 'chai';
import ModalManager from '@restart/ui/ModalManager';
import { fireEvent, render } from '@testing-library/react';
Expand Down Expand Up @@ -286,4 +287,32 @@ describe('<Offcanvas>', () => {
const offcanvasElem = getByTestId('test');
expect(offcanvasElem.getAttribute('role')).to.not.exist;
});

it('should not mount, unmount and mount content on show', () => {
const InnerComponent = ({ onMount, onUnmount }) => {
useEffect(() => {
onMount();
return () => {
onUnmount();
};
}, []);

return <div>Content</div>;
};

const onMountSpy = sinon.spy();
const onUnmountSpy = sinon.spy();

const { unmount } = render(
<Offcanvas data-testid="test" onHide={noop} show>
<InnerComponent onMount={onMountSpy} onUnmount={onUnmountSpy} />
</Offcanvas>,
);

onMountSpy.callCount.should.equal(1);

unmount();

onUnmountSpy.callCount.should.equal(1);
});
});