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

Enhance transition components #996

Merged
merged 5 commits into from
Apr 27, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/feedback/Dialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ const Dialog = function Dialog({

return (
<Wrapper
visible={transitionComponentVisible}
direction={transitionComponentVisible ? 'in' : 'out'}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious: this Wrapper may be a reference to a TransitionComponent functional component or it might be a Fragment. Do Fragments happily take any props without grumbling? Presumably typechecking would bark if not...I just haven't thought about it before.

Copy link
Contributor Author

@acelaya acelaya Apr 26, 2023

Choose a reason for hiding this comment

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

Yeah, I thought the same at first, but not only it does not complain when type-checking, but the IDE properly autocompletes valid props, and complains if you set something not valid for TransitionComponent.

image

So I guess the type of Fragment is defined in a way that it accepts anything, knowing it will be actually "lost".

onTransitionEnd={onTransitionEnd}
>
<div
Expand Down
4 changes: 2 additions & 2 deletions src/components/feedback/test/Dialog-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ const createComponent = (Component, props = {}) => {
/**
* @type {import('../../../types').TransitionComponent}
*/
const ComponentWithTransition = ({ children, visible, onTransitionEnd }) => {
const ComponentWithTransition = ({ children, direction, onTransitionEnd }) => {
// Fake a 50ms transition time
setTimeout(() => onTransitionEnd?.(visible ? 'in' : 'out'), 50);
setTimeout(() => onTransitionEnd?.(direction), 50);
return <div>{children}</div>;
};

Expand Down
8 changes: 4 additions & 4 deletions src/components/transition/Slider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import type { TransitionComponent } from '../../types';

const Slider: TransitionComponent = ({
children,
visible,
direction = 'in',
onTransitionEnd,
}) => {
const visible = direction === 'in';
const containerRef = useRef<HTMLDivElement | null>(null);
const [containerHeight, setContainerHeight] = useState(visible ? 'auto' : 0);

Expand Down Expand Up @@ -55,15 +56,14 @@ const Slider: TransitionComponent = ({
const handleTransitionEnd = useCallback(() => {
if (visible) {
setContainerHeight('auto');
onTransitionEnd?.('in');
} else {
// When the collapse animation completes, stop rendering the content so
// that the browser has fewer nodes to render and the content is removed
// from keyboard navigation.
setContentVisible(false);
onTransitionEnd?.('out');
}
}, [setContainerHeight, visible, onTransitionEnd]);
onTransitionEnd?.(direction);
}, [setContainerHeight, visible, onTransitionEnd, direction]);

const isFullyVisible = containerHeight === 'auto';

Expand Down
58 changes: 30 additions & 28 deletions src/components/transition/test/Slider-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ describe('Slider', () => {

const createSlider = (props = {}) => {
return mount(
<Slider visible={false} {...props}>
<Slider {...props}>
<div style={{ width: 100, height: 200 }}>Test content</div>
</Slider>,
{ attachTo: container }
Expand All @@ -24,8 +24,8 @@ describe('Slider', () => {
container.remove();
});

it('should render collapsed if `visible` is false on mount', () => {
const wrapper = createSlider({ visible: false });
it('should render collapsed if `direction` is `out` on mount', () => {
const wrapper = createSlider({ direction: 'out' });
const { height } = wrapper.getDOMNode().getBoundingClientRect();
assert.equal(height, 0);

Expand All @@ -34,25 +34,27 @@ describe('Slider', () => {
assert.equal(wrapper.getDOMNode().style.display, 'none');
});

it('should render expanded if `visible` is true on mount', () => {
const wrapper = createSlider({ visible: true });
const { height } = wrapper.getDOMNode().getBoundingClientRect();
assert.equal(height, 200);
['in', undefined].forEach(direction => {
it('should render expanded if `direction` is `in` or not provided on mount', () => {
const wrapper = createSlider({ direction });
const { height } = wrapper.getDOMNode().getBoundingClientRect();
assert.equal(height, 200);
});
});

it('should transition to expanded if `visible` changes to `true`', () => {
const wrapper = createSlider({ visible: false });
it('should transition to expanded if `direction` changes to `in`', () => {
const wrapper = createSlider({ direction: 'out' });

wrapper.setProps({ visible: true });
wrapper.setProps({ direction: 'in' });

const containerStyle = wrapper.getDOMNode().style;
assert.equal(containerStyle.height, '200px');
});

it('should transition to collapsed if `visible` changes to `false`', done => {
const wrapper = createSlider({ visible: true });
it('should transition to collapsed if `direction` changes to `out`', done => {
const wrapper = createSlider({ direction: 'in' });

wrapper.setProps({ visible: false });
wrapper.setProps({ direction: 'out' });

setTimeout(() => {
const containerStyle = wrapper.getDOMNode().style;
Expand All @@ -62,9 +64,9 @@ describe('Slider', () => {
});

it('should set the container height to "auto" when an expand transition finishes', () => {
const wrapper = createSlider({ visible: false });
const wrapper = createSlider({ direction: 'out' });

wrapper.setProps({ visible: true });
wrapper.setProps({ direction: 'in' });

let containerStyle = wrapper.getDOMNode().style;
assert.equal(containerStyle.height, '200px');
Expand All @@ -75,38 +77,38 @@ describe('Slider', () => {
assert.equal(containerStyle.height, 'auto');
});

it('should hide overflowing content when not fully visible', () => {
it('should hide overflowing content when not fully expanded', () => {
// When fully collapsed, overflow should be hidden.
const wrapper = createSlider({ visible: false });
const wrapper = createSlider({ direction: 'out' });
let containerStyle = wrapper.getDOMNode().style;
assert.equal(containerStyle.overflow, 'hidden');

// When starting to expand, or when collapsing, overflow should also be hidden.
wrapper.setProps({ visible: true });
wrapper.setProps({ direction: 'in' });
assert.equal(containerStyle.overflow, 'hidden');

// When fully visible, we make overflow visible to make focus rings or
// When fully expanded, we make overflow visible to make focus rings or
// other content which extends beyond the bounds of the Slider visible.
wrapper.find('div').first().simulate('transitionend');
containerStyle = wrapper.getDOMNode().style;
assert.equal(containerStyle.overflow, 'visible');
});

it('should stop rendering content when a collapse transition finishes', () => {
const wrapper = createSlider({ visible: true });
const wrapper = createSlider({ direction: 'in' });

wrapper.setProps({ visible: false });
wrapper.setProps({ direction: 'out' });

wrapper.find('div').first().simulate('transitionend');

const containerStyle = wrapper.getDOMNode().style;
assert.equal(containerStyle.display, 'none');
});

[true, false].forEach(visible => {
['in', 'out'].forEach(direction => {
it('should handle unmounting while expanding or collapsing', () => {
const wrapper = createSlider({ visible });
wrapper.setProps({ visible: !visible });
const wrapper = createSlider({ direction });
wrapper.setProps({ direction: direction === 'in' ? 'out' : 'in' });
wrapper.unmount();
});
});
Expand All @@ -115,12 +117,12 @@ describe('Slider', () => {
'should pass a11y checks',
checkAccessibility([
{
name: 'visible',
content: () => createSlider({ visible: true }),
name: 'in',
content: () => createSlider({ direction: 'in' }),
},
{
name: 'hidden',
content: () => createSlider({ visible: false }),
name: 'out',
content: () => createSlider({ direction: 'out' }),
},
])
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ export default function UsingComponentsPage() {
<Library.Code
size="sm"
content={`type TransitionComponentProps = {
visible: boolean;
direction?: 'in' | 'out';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we document that default is in?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

onTransitionEnd?: (direction: 'in' | 'out') => void;
};`}
title="Common transition-component props"
Expand Down
21 changes: 11 additions & 10 deletions src/pattern-library/components/patterns/transition/SliderPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,17 @@ import Library from '../../Library';

const Slider_: FunctionComponent<
ComponentProps<TransitionComponent> & { _transitionStatus?: 'in' | 'out' }
> = ({ children, visible, onTransitionEnd, _transitionStatus }) => {
const [isVisible, setIsVisible] = useState(visible);
const toggleSlider = () => setIsVisible(prev => !prev);
> = ({ children, direction, onTransitionEnd, _transitionStatus }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This typing is a little hard to read (the multi-line wrapping). Is it possible to construct the type elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, this is now redundant, and _transitionStatus can be completely discarded in favor of direction. It was needed when visible was used, but not anymore.

This can be simplified as const Slider_: TransitionComponent = ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok no, my mistake. Doing that has other side effects.

I will extract the type in multiple definitions for better readability.

const [currentDirection, setCurrentDirection] = useState(direction);
const toggleSlider = () =>
setCurrentDirection(prev => (prev === 'in' ? 'out' : 'in'));

return (
<div className="flex-col w-full space-y-2">
<Button onClick={toggleSlider} variant="primary">
{isVisible ? 'Hide' : 'Show'} slider
{currentDirection === 'in' ? 'Hide' : 'Show'} slider
</Button>
<Slider visible={isVisible} onTransitionEnd={onTransitionEnd}>
<Slider direction={currentDirection} onTransitionEnd={onTransitionEnd}>
{children}
</Slider>
<div>
Expand Down Expand Up @@ -66,7 +67,7 @@ export default function SliderPage() {
<Library.Usage componentName="Slider" />
<Library.Example>
<Library.Demo title="Basic example" withSource>
<Slider_ visible={false}>
<Slider_ direction="out">
<Card>
<CardContent>This is the content of the Slider</CardContent>
</Card>
Expand All @@ -76,17 +77,17 @@ export default function SliderPage() {
</Library.Pattern>

<Library.Pattern title="Props">
<Library.Example title="visible">
This mandatory boolean prop tells if the Slider is currently
displayed or hidden.
<Library.Example title="direction">
This prop tells if the Slider is currently expanded (<code>in</code>
) or collapsed (<code>out</code>). It is <code>in</code> by default.
Comment on lines +87 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

A task for me, especially now that we have reliable routing and fragment navigation, is to create a better centralized documentation for common component props. That is, this documentation probably shouldn't be necessary specifically for Slider, instead, we should be able to reuse or link to generalized documentation for this prop.

</Library.Example>

<Library.Example title="onTransitionEnd">
Optionally, you can provide a callback that will be invoked once the{' '}
<code>in</code>/<code>out</code> transitions end.
<Library.Demo title="Slider with onTransitionEnd" withSource>
<Slider_
visible={false}
direction="out"
onTransitionEnd={direction => setTransitionStatus(direction)}
_transitionStatus={transitionStatus}
>
Expand Down
2 changes: 1 addition & 1 deletion src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,6 @@ export type IconComponent = FunctionComponent<JSX.SVGAttributes<SVGSVGElement>>;
* animate the mounting and unmounting of a child component.
*/
export type TransitionComponent = FunctionComponent<{
visible: boolean;
direction?: 'in' | 'out';
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 mean that all TransitionComponents have to support both an in and an out transition? Is this an OK assumption to make?

onTransitionEnd?: (direction: 'in' | 'out') => void;
}>;