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

Panel: Improve scroll view handling when expanding #23327

Merged
merged 34 commits into from
Jul 14, 2020
Merged

Panel: Improve scroll view handling when expanding #23327

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
cb813a1
Add scrollIntoView interaction for PanelBody
Jun 19, 2020
5cddfb0
Adjust PanelBody story to include scrollIntoView example
Jun 19, 2020
7311fb7
Adjust bottom padding for Layout sidebar
Jun 19, 2020
7619ffd
Allow for opened prop to control disclosure state
Jun 19, 2020
546a846
Update PanelBody tests
Jun 19, 2020
ff1c7cf
Update snapshots
Jun 19, 2020
1f183ac
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jun 22, 2020
fa31367
Fix tests for PanelColorSettings + Update snapshots
Jun 22, 2020
d1fed38
Add new PanelBody props to README
Jun 22, 2020
3c8326c
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jun 23, 2020
29a4949
Add react-merge-refs dependency
Jun 23, 2020
5e82406
Use react-merge-refs util
Jun 23, 2020
db594d7
Update test snapshots
Jun 23, 2020
740f69e
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jun 23, 2020
ff7b3a6
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jun 24, 2020
277f242
Remove disableSmoothScrollIntoView prop from PanelBody
Jun 24, 2020
85d4384
Update snapshots
Jun 24, 2020
09776c1
Remove focusable prop
Jun 24, 2020
1425782
Remove use-combined-refs - No longer needed
Jun 24, 2020
73783b1
Remove onToggle from useUpdateEffect deps array
Jun 24, 2020
e61bb11
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jun 26, 2020
444bc3a
Adjust onToggle callback handling
Jun 26, 2020
a2dfe87
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jun 26, 2020
8c4f6a6
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jun 29, 2020
a194641
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jun 30, 2020
2e5b430
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jul 6, 2020
1c1ff72
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jul 7, 2020
fb33dc0
Remove Reakit/Disclosure integration (for now)
Jul 7, 2020
c50d006
Update PanelBody tests
Jul 7, 2020
10fdadc
Update snapshots
Jul 7, 2020
fe70a9a
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jul 7, 2020
d7c22a0
Merge branch 'master' into update/panel-improve-scroll-view-handling
Jul 8, 2020
45510e7
Fix initialOpen handling for PanelBody
Jul 8, 2020
8a35791
Remove unnecessary act wrappers in panel body tests
Jul 13, 2020
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
200 changes: 93 additions & 107 deletions packages/block-editor/src/components/panel-color-settings/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { create, act } from 'react-test-renderer';
import { render } from '@testing-library/react';
import { noop } from 'lodash';

/**
Expand All @@ -11,120 +11,106 @@ import PanelColorSettings from '../';

describe( 'PanelColorSettings', () => {
it( 'should not render anything if there are no colors to choose', async () => {
let root;

await act( async () => {
root = create(
<PanelColorSettings
title="Test Title"
colors={ [] }
disableCustomColors={ true }
colorSettings={ [
{
value: '#000',
onChange: noop,
label: 'border color',
},
{
value: '#111',
onChange: noop,
label: 'background color',
},
] }
/>
);
} );

expect( root.toJSON() ).toBe( null );
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 () => {
let root;
await act( async () => {
root = create(
<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( root ).not.toBe( null );
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 () => {
let root;
await act( async () => {
root = create(
<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( root ).not.toBe( null );
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 () => {
let root;
await act( async () => {
root = create(
<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( root ).not.toBe( null );
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( '' );
} );
} );
14 changes: 14 additions & 0 deletions packages/components/src/panel/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,20 @@ The class that will be added with `components-panel__body`, if the panel is curr
- Type: `String`
- Required: No

###### disableSmoothScrollIntoView

By default, the `PanelBody` will smooth scroll into view (if needed). Smooth scrolling can be disabled if `disableSmoothScrollIntoView` is set to `true`.

- Type: `Boolean`
- Required: No

###### focusable

By default, the `PanelBody` header is focusable. This can be disabled if `focusable` is set to `false`.

- Type: `Boolean`
- Required: No

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we really need these props as part of the API or should we just be opinionated and avoid more APIs to support?

Copy link
Author

Choose a reason for hiding this comment

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

@youknowriad I think we can be more opinionated. Initially, I wasn't sure how users may respond to the new smooth scrolling behaviour. If it is problematic, we can disable it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about the focusable prop?

Copy link
Member

Choose a reason for hiding this comment

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

I see that the focusable prop is passed to Disclosure underneath. It probably doesn't work as you're expecting. It just guarantees that, when true, the element will remain focusable even when the element is disabled. It's the same as our Button's isFocusable prop:

gutenberg/packages/components/src/button/index.js

Line 43 in b98418d

__experimentalIsFocusable: isFocusable,

If this causes confusion, that's a good signal that we should find a more descriptive name for the prop. Something like isFocusableWhenDisabled.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm! I'm that case, I'll remove for now :)

###### icon

An icon to be shown next to the `PanelBody` title.
Expand Down
Loading