Panel: Improve scroll view handling when expanding #23327
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,94 +2,139 @@ | |
| * External dependencies | ||
| */ | ||
| import classnames from 'classnames'; | ||
| import { noop } from 'lodash'; | ||
| import { | ||
| useDisclosureState, | ||
| Disclosure, | ||
| DisclosureContent, | ||
| } from 'reakit/Disclosure'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { useReducedMotion } from '@wordpress/compose'; | ||
| import { forwardRef, useRef } from '@wordpress/element'; | ||
| import { chevronUp, chevronDown } from '@wordpress/icons'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import Button from '../button'; | ||
| import Icon from '../icon'; | ||
| import { useCombinedRefs, useUpdateEffect } from '../utils'; | ||
|
|
||
| export function PanelBody( | ||
| { | ||
| children, | ||
| className, | ||
| disableSmoothScrollIntoView, | ||
| focusable, | ||
| icon, | ||
| initialOpen: initialOpenProp, | ||
| onToggle = noop, | ||
| opened, | ||
| title, | ||
| }, | ||
| ref | ||
| ) { | ||
| const initialOpen = useRef( initialOpenProp ).current; | ||
| const disclosure = useDisclosureState( { | ||
| visible: initialOpen !== undefined ? initialOpen : opened, | ||
| } ); | ||
| const nodeRef = useRef(); | ||
| const combinedRefs = useCombinedRefs( ref, nodeRef ); | ||
|
|
||
| // Defaults to 'smooth' scrolling | ||
| // https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView | ||
| let scrollBehavior = useReducedMotion() ? 'auto' : 'smooth'; | ||
|
|
||
| // However, this behavior can be overridden by prop | ||
| if ( disableSmoothScrollIntoView ) { | ||
| scrollBehavior = 'auto'; | ||
| } | ||
|
|
||
| const isOpened = disclosure.visible; | ||
|
|
||
| // Runs after initial render | ||
| useUpdateEffect( () => { | ||
| if ( disclosure.visible ) { | ||
| /* | ||
| * Scrolls the content into view when visible. | ||
| * This improves the UX when there are multiple stacking <PanelBody /> | ||
| * components in a scrollable container. | ||
| */ | ||
| if ( nodeRef.current.scrollIntoView ) { | ||
| nodeRef.current.scrollIntoView( { | ||
| inline: 'nearest', | ||
| block: 'nearest', | ||
| behavior: scrollBehavior, | ||
| } ); | ||
| } | ||
| } | ||
|
|
||
| onToggle( disclosure.visible ); | ||
| }, [ disclosure.visible, onToggle, scrollBehavior ] ); | ||
|
There was a problem hiding this comment. The <PanelBody onToggle={() => {}} />The user would have to do this: const onToggle = useCallback(() => {}, []);
<PanelBody onToggle={onToggle} />Is this intended? There was a problem hiding this comment. @diegohaz Thanks for pointing this out! I'll remove it from the deps array There was a problem hiding this comment. I think removing from the deps array is not enough as you could end up with a component calling an old version of the "onToggle" prop. I solve this using refs personally. Example https://github.com/WordPress/gutenberg/pull/23394/files#diff-9db94cc958a10efcb72038925b6bc24cR57-R60 There was a problem hiding this comment. Hmm! Interesting :). Thanks for the suggestion! If this pattern continues to pop up, maybe we can create a util for this effect :D There was a problem hiding this comment. I use this helper on Reakit. The difference is that it's |
||
|
|
||
| useUpdateEffect( () => { | ||
| disclosure.setVisible( opened ); | ||
| }, [ disclosure.setVisible, opened ] ); | ||
|
There was a problem hiding this comment. For me using effects to sync between disclosure state and external state doesn't seems like a great API. It seems thought l like we're forced to do it this way because of how Reakit disclosure API is modeled. I wonder personally if the Reakit Disclosure API could be made more friendly for "controlled components". cc @diegohaz There was a problem hiding this comment.
If you use function OptionallyControlledComponent(props) {
const [visible, setVisible] = React.useState(props.visible);
if (props.visible !== undefined && props.visible !== visible) {
// React will bail out from rendering since this is called in the render phase
setVisible(props.visible);
}
...
}This is basically function OptionallyControlledComponent(props) {
const disclosure = useDisclosureState({ visible: props.visible });
if (props.visible !== undefined && props.visible !== disclosure.visible) {
disclosure.setVisible(props.visible);
}
return (
<>
<Disclosure {...disclosure}>Toggle</Disclosure>
<DisclosureContent {...disclosure}>Content</DisclosureContent>
</>
);
}In both cases above you would be able to pass a <OptionallyControlledComponent visible />Or pass nothing, and this would be an uncontrolled component: <OptionallyControlledComponent />You can even add a <OptionallyControlledComponent defaultVisible />But you can always opt out from using the state hook inside the component and make it fully controlled by passing the required state props to the components: function ControlledComponent(props) {
const baseId = useInstanceId();
return (
<>
<Disclosure
visible={props.visible}
baseId={baseId}
toggle={props.onToggle}
>
Toggle
</Disclosure>
<DisclosureContent baseId={baseId} visible={props.visible}>Content</DisclosureContent>
</>
);
}You can find the list of state props here:
Unfortunately, it's still not showing which props are required and optional. This is something we should improve in the documentation. But, if the editor supports TypeScript (like VSCode), you'll see the required props without the
Here's a CodeSandbox with some examples: https://codesandbox.io/s/controlled-and-uncontrolled-reakit-components-rgwck That said, it's not all set in stone. There's an open issue about this (ariakit/ariakit#487). Because of the consistency with There was a problem hiding this comment. @diegohaz I just tried out the
(The GIF doesn't fully capture it. It was flipping REALLY fast) The component seems to be pretty happy with the useUpdateEffect( () => {
disclosure.setVisible( opened );
}, [ disclosure.setVisible, opened ] );I hear where @youknowriad is coming from. With that said, I'm okay with the current implementation, with the idea of smoothening it out later. I think my upcoming updates for the That being part of this PR: There was a problem hiding this comment. Just to clarify, i'm ok with the current implementation too but I do believe both useState and useDisclosureState favor local state where in our components (and I believe most UI libraries but I'm not sure), we don't build "optionally controlled components", I feel we default to "controlled components" and the uncontrolled ones are the exception. In pure React, you don't need useState and effects to achieve that you just pass the props down. There was a problem hiding this comment. @ItsJonQ I believe it's because you're calling Take a look at the examples here: https://codesandbox.io/s/controlled-and-uncontrolled-reakit-components-rgwck |
||
|
|
||
| const classes = classnames( 'components-panel__body', className, { | ||
| 'is-opened': isOpened, | ||
| } ); | ||
|
|
||
| return ( | ||
| <div className={ classes } ref={ combinedRefs }> | ||
| <PanelBodyTitle | ||
| focusable={ focusable } | ||
| title={ title } | ||
| icon={ icon } | ||
| { ...disclosure } | ||
| /> | ||
| <DisclosureContent { ...disclosure }> | ||
| { children } | ||
| </DisclosureContent> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const PanelBodyTitle = forwardRef( | ||
| ( { focusable, isOpened, icon, title, ...props }, ref ) => { | ||
| if ( ! title ) return null; | ||
|
|
||
| return ( | ||
| <h2 className="components-panel__body-title"> | ||
| <Disclosure | ||
| as={ Button } | ||
| className="components-panel__body-toggle" | ||
| focusable={ focusable } | ||
| ref={ ref } | ||
| { ...props } | ||
| > | ||
| { /* | ||
| Firefox + NVDA don't announce aria-expanded because the browser | ||
| repaints the whole element, so this wrapping span hides that. | ||
| */ } | ||
| <span aria-hidden="true"> | ||
| <Icon | ||
| className="components-panel__arrow" | ||
| icon={ isOpened ? chevronUp : chevronDown } | ||
| /> | ||
| </span> | ||
| { title } | ||
| { icon && ( | ||
| <Icon | ||
| icon={ icon } | ||
| className="components-panel__icon" | ||
| size={ 20 } | ||
| /> | ||
| ) } | ||
| </Disclosure> | ||
| </h2> | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| const ForwardedComponent = forwardRef( PanelBody ); | ||
|
|
||
| export default ForwardedComponent; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has an open PR here for the refactor #23065 is this based on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh! Not at all. I didn't know that https://github.com/WordPress/gutenberg/pull/23065/files existed. I refactored in order to integrate with Reakit