Panel: Improve scroll view handling when expanding #23327
Conversation
Refactor interactions with Reakit/Disclosure. Add custom hooks for ref and rendering handling.
ItsJonQ
added
[Type] Enhancement
| @@ -59,6 +59,7 @@ | |||
|
|
|||
| .interface-interface-skeleton__sidebar > div { | |||
| height: 100%; | |||
| padding-bottom: $grid-unit-60; | |||
There was a problem hiding this comment.
Adds some extra padding at the bottom. Panels near won't feel so "tight"
There was a problem hiding this comment.
Is this specific to the edit-post sidebar or all interface sidebars.
I know this selector was already here but it seems like it's too generic while added in edit-post?
There was a problem hiding this comment.
I suppose all Interface sidebars. I added it here for now, as I wasn't familiar with the Interface components, and wanted to limit scope. (Also, the CSS selector already existed)
|
Size Change: +2.39 kB (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
That's super weird! Couldn't find any problem in the implementation. I suspect it has something to do with Code/**
* External dependencies
*/
import { render } from '@testing-library/react';
import { noop } from 'lodash';
/**
* Internal dependencies
*/
import PanelColorSettings from '../';
describe( 'PanelColorSettings', () => {
it( 'should not render anything if there are no colors to choose', async () => {
const { container } = render(
<PanelColorSettings
title="Test Title"
colors={ [] }
disableCustomColors={ true }
colorSettings={ [
{
value: '#000',
onChange: noop,
label: 'border color',
},
{
value: '#111',
onChange: noop,
label: 'background color',
},
] }
/>
);
expect( container.innerHTML ).toBe( '' );
} );
it( 'should render a color panel if at least one setting supports custom colors', async () => {
const { container } = render(
<PanelColorSettings
title="Test Title"
colors={ [] }
disableCustomColors={ true }
colorSettings={ [
{
value: '#000',
onChange: noop,
label: 'border color',
},
{
value: '#111',
onChange: noop,
label: 'background color',
disableCustomColors: false,
},
] }
/>
);
expect( container.innerHTML ).not.toBe( '' );
} );
it( 'should render a color panel if at least one setting specifies some colors to choose', async () => {
const { container } = render(
<PanelColorSettings
title="Test Title"
colors={ [] }
disableCustomColors={ true }
colorSettings={ [
{
value: '#000',
onChange: noop,
label: 'border color',
colors: [
{
slug: 'red',
name: 'Red',
color: '#ff0000',
},
],
},
{
value: '#111',
onChange: noop,
label: 'background color',
},
] }
/>
);
expect( container.innerHTML ).not.toBe( '' );
} );
it( 'should not render anything if none of the setting panels has colors to choose', async () => {
const { container } = render(
<PanelColorSettings
title="Test Title"
colors={ [] }
disableCustomColors={ false }
colorSettings={ [
{
value: '#000',
onChange: noop,
label: 'border color',
colors: [],
disableCustomColors: true,
},
{
value: '#111',
onChange: noop,
label: 'background color',
colors: [],
disableCustomColors: true,
},
] }
/>
);
expect( container.innerHTML ).not.toBe( '' );
} );
} ); |
|
@diegohaz Thank you so much for digging into this! I apologize for the extra work. I guess I was thrown off by the Reakit warnings, and focused on that (with Really appreciate you taking the time to help! <3 |
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten,
ellatrix and
youknowriad
as code owners
| * Source: | ||
| * https://github.com/reakit/reakit/blob/master/packages/reakit-utils/src/useUpdateEffect.ts | ||
| */ | ||
| export function useUpdateEffect( effect, deps ) { |
There was a problem hiding this comment.
Do we use this technique in other parts of our code base? would it make sense to move it to compose package or is it too soon for it to be a public API?
There was a problem hiding this comment.
I believe I may have used it in my component work. I know I've had to use it many times in other projects. I chatted about it briefly with @diegohaz . It sounds like he shares the same sentiment, which is why it exists as a Reakit util I suppose :).
I'm be open to adding it to @wordpress/compose.
Side question... I'm starting to build up a collection of utils within @wordpress/components. There are more that I'd like to add/abstract. In the future, would some of these migrate to something like @wordpress/compose? Or would we create a new dedicated package for these JS/component utils?
Thanks!
There was a problem hiding this comment.
I think compose makes sense for React hooks. The question becomes when are we confident that we'll maintain backward compatibility forever. So it seems to me starting as internal to components and moving to compose when we think they're stable makes sense.
Now, for other kind of utils, i'd say it depends, we generally avoid generic packages like "utils" or "helpers" or things like that and prefer more dedicated packages "url", "priority-query"...
| @@ -59,6 +59,7 @@ | |||
|
|
|||
| .interface-interface-skeleton__sidebar > div { | |||
| height: 100%; | |||
| padding-bottom: $grid-unit-60; | |||
There was a problem hiding this comment.
Is this specific to the edit-post sidebar or all interface sidebars.
I know this selector was already here but it seems like it's too generic while added in edit-post?
| opened: props.initialOpen === undefined ? true : props.initialOpen, | ||
| }; | ||
| this.toggle = this.toggle.bind( this ); | ||
| export function PanelBody( |
There was a problem hiding this comment.
This has an open PR here for the refactor #23065 is this based on it?
There was a problem hiding this comment.
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
|
Update! Unfortunately, I've removed the Note: I'm 💯 in favour of unique IDs + aria association. That's how it should be done. I think we can reserve the Reakit integration for another PR 🙏 Thank you @youknowriad + @diegohaz for your time in reviewing this one! |
In this case you can explicitly set gutenberg/packages/components/src/toolbar/toolbar-container.js Lines 18 to 20 in 0506ca1 gutenberg/packages/components/src/toolbar/stories/index.js Lines 43 to 47 in f237978 I'll review the changes this week. Thanks for working on this! ❤️ |
|
Thanks for the review @diegohaz + @youknowriad !! Will merge this up |
|
This change removed the Will create a new issue... I do realize it was removed accidentally, this kind of things can happen to everyone. It would be great to have tools in place to avoid this kind of regressions though. |
|
@afercia Oh! Thank you for catching this 🙏 . |
This update adds an interaction for
<PanelBody />that scrolls the contents into view (if needed). This improves UX within the Editor's sidebar, especially for collapsed items that are nearer to the bottom.How has this been tested?
npm run devTypes of changes
PanelBodyinto a functional componentReakit/Disclosurefor a11y + interaction handling of collapse/expandelement.scrollIntoView()for native (browser) scroll handlingrefhandlingChecklist: