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

Tabs: indicator animation #60560

Merged
merged 27 commits into from
May 23, 2024
Merged

Tabs: indicator animation #60560

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
898f29d
Initial tab indicator animation implementation
DaniGuardiola Apr 8, 2024
ce628d3
Add changelog entry
DaniGuardiola Apr 8, 2024
7e43c02
Minor tweak
DaniGuardiola Apr 8, 2024
47f029d
Merge branch 'trunk' into tabs-animation
DaniGuardiola Apr 8, 2024
0a66bbd
Fix downstream issues.
DaniGuardiola Apr 12, 2024
3dae51c
Use ResizeObserver.
DaniGuardiola Apr 12, 2024
a0b0581
Add width transition.
DaniGuardiola Apr 12, 2024
a5d5d27
Simplify and use framer motion
DaniGuardiola Apr 17, 2024
22909d8
vertical indicator
DaniGuardiola Apr 17, 2024
77d208f
Revert to previous implementation.
DaniGuardiola Apr 22, 2024
e9ccc02
Fix bug due to some animations breaking measurement of the tab element.
DaniGuardiola Apr 22, 2024
283f109
Abstracted and fixed all previous issues.
DaniGuardiola Apr 22, 2024
b3ab95f
Follow naming convention for classes.
DaniGuardiola Apr 23, 2024
45d8215
Support vertical orientation + misc fixes and improvements.
DaniGuardiola Apr 24, 2024
f364cc2
Clean up styles a bit.
DaniGuardiola Apr 24, 2024
f0a5a65
Merge branch 'trunk' into tabs-animation
DaniGuardiola Apr 24, 2024
539be1e
Better focus ring animation + minor style cleanup.
DaniGuardiola Apr 24, 2024
fa9f34e
Fix changelog (oops).
DaniGuardiola Apr 24, 2024
16fac9f
Actually fix changelog.
DaniGuardiola Apr 24, 2024
6b2f541
Remove deprecated `reduceMotion` utility.
DaniGuardiola Apr 26, 2024
aa6e66b
Merge branch 'trunk' into tabs-animation
DaniGuardiola May 22, 2024
2f14f6d
Fix open/closed
DaniGuardiola May 22, 2024
02099ac
Add vertical tabs story
DaniGuardiola May 22, 2024
2c4d085
Move ResizeObserver unobserve to effect cleanup
DaniGuardiola May 22, 2024
59523e4
Merge branch 'trunk' of https://github.com/WordPress/gutenberg into t…
DaniGuardiola May 23, 2024
bed8a36
Remove outdated type cast.
DaniGuardiola May 23, 2024
f0b896b
Hide vertical indicator for now.
DaniGuardiola May 23, 2024
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

### Enhancements

- `Tabs`: Animate indicator ([#60560](https://github.com/WordPress/gutenberg/pull/60560)).
- `ComboboxControl`: Introduce Combobox expandOnFocus prop ([#61705](https://github.com/WordPress/gutenberg/pull/61705)).

### Bug Fixes
Expand Down
14 changes: 14 additions & 0 deletions packages/components/src/tabs/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,20 @@ const Template: StoryFn< typeof Tabs > = ( props ) => {

export const Default = Template.bind( {} );

const VerticalTemplate: StoryFn< typeof Tabs > = ( props ) => {
return (
<Tabs orientation="vertical" { ...props }>
<Tabs.TabList style={ { maxWidth: '10rem' } }>
<Tabs.Tab tabId="tab1">Tab 1</Tabs.Tab>
<Tabs.Tab tabId="tab2">Tab 2</Tabs.Tab>
<Tabs.Tab tabId="tab3">Tab 3</Tabs.Tab>
</Tabs.TabList>
</Tabs>
);
};

export const Vertical = VerticalTemplate.bind( {} );

const DisabledTabTemplate: StoryFn< typeof Tabs > = ( props ) => {
return (
<Tabs { ...props }>
Expand Down
77 changes: 43 additions & 34 deletions packages/components/src/tabs/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,49 @@ import * as Ariakit from '@ariakit/react';
*/
import { COLORS } from '../utils';
import { space } from '../utils/space';
import { reduceMotion } from '../utils/reduce-motion';

export const TabListWrapper = styled.div`
position: relative;
DaniGuardiola marked this conversation as resolved.
Show resolved Hide resolved
display: flex;
align-items: stretch;
flex-direction: row;
&[aria-orientation='vertical'] {
flex-direction: column;
}
@media not ( prefers-reduced-motion: reduce ) {
&.is-animation-enabled::after {
transition-property: left, top, width, height;
transition-duration: 0.2s;
transition-timing-function: ease-out;
}
}
&::after {
DaniGuardiola marked this conversation as resolved.
Show resolved Hide resolved
content: '';
position: absolute;
pointer-events: none;

// Windows high contrast mode.
outline: 2px solid transparent;
outline-offset: -1px;
}
&:not( [aria-orientation='vertical'] )::after {
left: var( --indicator-left );
bottom: 0;
width: var( --indicator-width );
height: 0;
border-bottom: var( --wp-admin-border-width-focus ) solid
${ COLORS.theme.accent };
}
&[aria-orientation='vertical']::after {
DaniGuardiola marked this conversation as resolved.
Show resolved Hide resolved
/* Temporarily hidden, context: https://github.com/WordPress/gutenberg/pull/60560#issuecomment-2126670072 */
opacity: 0;

right: 0;
top: var( --indicator-top );
height: var( --indicator-height );
border-right: var( --wp-admin-border-width-focus ) solid
${ COLORS.theme.accent };
}
`;

export const Tab = styled( Ariakit.Tab )`
Expand Down Expand Up @@ -51,34 +85,6 @@ export const Tab = styled( Ariakit.Tab )`
outline: none;
}

// Tab indicator
&::after {
content: '';
position: absolute;
right: 0;
bottom: 0;
left: 0;
pointer-events: none;

// Draw the indicator.
background: ${ COLORS.theme.accent };
height: calc( 0 * var( --wp-admin-border-width-focus ) );
border-radius: 0;

// Animation
transition: all 0.1s linear;
${ reduceMotion( 'transition' ) };
}

// Active.
&[aria-selected='true']::after {
height: calc( 1 * var( --wp-admin-border-width-focus ) );

// Windows high contrast mode.
outline: 2px solid transparent;
outline-offset: -1px;
}

// Focus.
&::before {
content: '';
Expand All @@ -90,17 +96,20 @@ export const Tab = styled( Ariakit.Tab )`
pointer-events: none;

// Draw the indicator.
box-shadow: 0 0 0 0 transparent;
box-shadow: 0 0 0 var( --wp-admin-border-width-focus )
${ COLORS.theme.accent };
border-radius: 2px;

// Animation
transition: all 0.1s linear;
${ reduceMotion( 'transition' ) };
opacity: 0;

@media not ( prefers-reduced-motion ) {
transition: opacity 0.1s linear;
}
}

&:focus-visible::before {
box-shadow: 0 0 0 var( --wp-admin-border-width-focus )
${ COLORS.theme.accent };
opacity: 1;

// Windows high contrast mode.
outline: 2px solid transparent;
Expand Down
133 changes: 130 additions & 3 deletions packages/components/src/tabs/tablist.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,13 @@ import * as Ariakit from '@ariakit/react';
* WordPress dependencies
*/
import warning from '@wordpress/warning';
import { forwardRef } from '@wordpress/element';
import {
forwardRef,
useEffect,
useLayoutEffect,
useRef,
useState,
} from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -17,19 +23,121 @@ import type { TabListProps } from './types';
import { useTabsContext } from './context';
import { TabListWrapper } from './styles';
import type { WordPressComponentProps } from '../context';
import clsx from 'clsx';

function useTrackElementOffset(
targetElement?: HTMLElement | null,
onUpdate?: () => void
) {
const [ indicatorPosition, setIndicatorPosition ] = useState( {
left: 0,
top: 0,
width: 0,
height: 0,
} );

// TODO: replace with useEventCallback or similar when officially available.
const updateCallbackRef = useRef( onUpdate );
useLayoutEffect( () => {
updateCallbackRef.current = onUpdate;
} );

const observedElementRef = useRef< HTMLElement >();
const resizeObserverRef = useRef< ResizeObserver >();
useEffect( () => {
if ( targetElement === observedElementRef.current ) {
return;
}

observedElementRef.current = targetElement ?? undefined;

function updateIndicator( element: HTMLElement ) {
setIndicatorPosition( {
left: element.offsetLeft,
top: element.offsetTop,
width: element.offsetWidth,
height: element.offsetHeight,
} );
updateCallbackRef.current?.();
Copy link
Member

Choose a reason for hiding this comment

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

What difference does it make if I just call onUpdate?.() here (and add it as a dependency of course)? It's not immediately clear why we need the updateCallbackRef in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similar logic to #60560 (comment), though in this case due to how the effect is implemented (idempotently) it wouldn't really break anything, but it would cause wasteful computation since there's an unnecessary dependency for the effect.

}

// Set up a ResizeObserver.
if ( ! resizeObserverRef.current ) {
DaniGuardiola marked this conversation as resolved.
Show resolved Hide resolved
resizeObserverRef.current = new ResizeObserver( () => {
if ( observedElementRef.current ) {
updateIndicator( observedElementRef.current );
}
} );
}
const { current: resizeObserver } = resizeObserverRef;

// Observe new element.
DaniGuardiola marked this conversation as resolved.
Show resolved Hide resolved
if ( targetElement ) {
updateIndicator( targetElement );
resizeObserver.observe( targetElement );
}

return () => {
// Unobserve previous element.
if ( observedElementRef.current ) {
resizeObserver.unobserve( observedElementRef.current );
}
};
}, [ targetElement ] );

return indicatorPosition;
}

type ValueUpdateContext< T > = {
previousValue: T;
};

function useOnValueUpdate< T >(
value: T,
onUpdate: ( context: ValueUpdateContext< T > ) => void
) {
const previousValueRef = useRef( value );

// TODO: replace with useEventCallback or similar when officially available.
const updateCallbackRef = useRef( onUpdate );
useLayoutEffect( () => {
updateCallbackRef.current = onUpdate;
} );

useEffect( () => {
if ( previousValueRef.current !== value ) {
updateCallbackRef.current( {
previousValue: previousValueRef.current,
} );
previousValueRef.current = value;
}
}, [ value ] );
}
Comment on lines +95 to +115
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to maintain the update callback in a ref? What difference would it make if I did:

Suggested change
function useOnValueUpdate< T >(
value: T,
onUpdate: ( context: ValueUpdateContext< T > ) => void
) {
const previousValueRef = useRef( value );
// TODO: replace with useEventCallback or similar when officially available.
const updateCallbackRef = useRef( onUpdate );
useLayoutEffect( () => {
updateCallbackRef.current = onUpdate;
} );
useEffect( () => {
if ( previousValueRef.current !== value ) {
updateCallbackRef.current( {
previousValue: previousValueRef.current,
} );
previousValueRef.current = value;
}
}, [ value ] );
}
function useOnValueUpdate< T >(
value: T,
onUpdate: ( context: ValueUpdateContext< T > ) => void
) {
const previousValueRef = useRef( value );
useLayoutEffect( () => {
if ( previousValueRef.current !== value ) {
onUpdate( {
previousValue: previousValueRef.current,
} );
previousValueRef.current = value;
}
}, [ value, onUpdate ] );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a low-level event-oriented hook that needs to fire only on value updates. If the callback was a dependency for the effect, passing a different callback would trigger it, ergo the hook would not be firing on value updates exclusively. That would be misleading, and would also break the logic of consumers. In our case, such scenario would cause the animation to be enabled at a time when it shouldn't be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, @jsnajdr also proposed this as I was about to push a commit with it, we had a parallel thinking moment :P #60560 (comment)


export const TabList = forwardRef<
HTMLDivElement,
WordPressComponentProps< TabListProps, 'div', false >
>( function TabList( { children, ...otherProps }, ref ) {
const context = useTabsContext();

const selectedId = context?.store.useState( 'selectedId' );
const indicatorPosition = useTrackElementOffset(
context?.store.item( selectedId )?.element
);

const [ animationEnabled, setAnimationEnabled ] = useState( false );
Copy link
Member

Choose a reason for hiding this comment

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

Looking at the code, I'd always expect animations. However, there are cases when animations won't be there, for example:

  • With reduced motion
  • For vertically oriented tabs

Should we document these and set the right expectations that animations are expected only for vertical tabs and when prefers-reduced-motion is not enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No animations with prefers-reduced-motion should be expected in every place where we have animations, and it's made explicit in the CSS. I don't personally feel like any further documentation is necessary in that front, unless we decide to do it consistently. However, we don't really have user-facing documentation in the first place for individual components anyway :P

Vertical tabs actually do have an animation. I might change this if we don't want the change (as discussed somewhere else in this review's comment), but currently it does, and it works in the same way except vertically (in the right side).

useOnValueUpdate(
selectedId,
( { previousValue } ) => previousValue && setAnimationEnabled( true )
);
DaniGuardiola marked this conversation as resolved.
Show resolved Hide resolved

if ( ! context ) {
warning( '`Tabs.TabList` must be wrapped in a `Tabs` component.' );
return null;
}
const { store } = context;

const { selectedId, activeId, selectOnMove } = store.useState();
const { activeId, selectOnMove } = store.useState();
const { setActiveId } = store;

const onBlur = () => {
Expand All @@ -50,9 +158,28 @@ export const TabList = forwardRef<
<Ariakit.TabList
ref={ ref }
store={ store }
render={ <TabListWrapper /> }
render={
<TabListWrapper
onTransitionEnd={ ( event ) => {
if ( event.pseudoElement === '::after' ) {
setAnimationEnabled( false );
}
} }
/>
}
onBlur={ onBlur }
{ ...otherProps }
style={ {
'--indicator-left': `${ indicatorPosition.left }px`,
'--indicator-top': `${ indicatorPosition.top }px`,
'--indicator-width': `${ indicatorPosition.width }px`,
'--indicator-height': `${ indicatorPosition.height }px`,
...otherProps.style,
} }
className={ clsx(
animationEnabled ? 'is-animation-enabled' : '',
otherProps.className
) }
>
{ children }
</Ariakit.TabList>
Expand Down
9 changes: 4 additions & 5 deletions packages/preferences/src/components/preferences-modal-tabs/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
$vertical-tabs-width: 160px;

.preferences__tabs-tablist {
DaniGuardiola marked this conversation as resolved.
Show resolved Hide resolved
position: absolute;
position: absolute !important;
DaniGuardiola marked this conversation as resolved.
Show resolved Hide resolved
top: $header-height + $grid-unit-30;
// Aligns button text instead of button box.
left: $grid-unit-20;
width: $vertical-tabs-width;

&::after {
Copy link
Member

Choose a reason for hiding this comment

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

Is this something we need to handle in the component itself? Perhaps if we provide an API to declare the indicator to be optional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems like an escape hatch and it's only used here. You could also say the same about everything else that's being customized or overriden here (a lot). I would advise against supporting this to prevent configuration-creep from building up. If this pattern becomes "official" or, at least, common enough, then I would agree.

At the root of this I think the problem is that Tabs from @wordpress/components is not what was needed here. Rather, it seems like we need tabs that look a certain way, and for that it seems like using Ariakit with the styles wanted for this interface is the way to go.

content: none !important;
}
}

.preferences__tabs-tab {
Expand All @@ -19,10 +22,6 @@ $vertical-tabs-width: 160px;
font-weight: 500;
}

&[aria-selected="true"]::after {
content: none;
}

&[role="tab"]:focus:not(:disabled) {
box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
// Windows high contrast mode.
Expand Down
Loading