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

Components: promote Scrollable #32446

Merged
merged 4 commits into from
Jun 7, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 6 additions & 0 deletions docs/manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -1103,6 +1103,12 @@
"markdown_source": "../packages/components/src/scroll-lock/README.md",
"parent": "components"
},
{
"title": "Scrollable",
"slug": "scrollable",
"markdown_source": "../packages/components/src/scrollable/README.md",
"parent": "components"
},
{
"title": "SelectControl",
"slug": "select-control",
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export { default as SelectControl } from './select-control';
export { default as Snackbar } from './snackbar';
export { default as SnackbarList } from './snackbar/list';
export { Spacer as __experimentalSpacer } from './spacer';
export { Scrollable as __experimentalScrollable } from './scrollable';
export { default as Spinner } from './spinner';
export { default as TabPanel } from './tab-panel';
export { Text as __experimentalText } from './text';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,27 +9,30 @@ This feature is still experimental. “Experimental” means this is an early im
## Usage

```jsx
import { Scrollable, View } from '@wordpress/components/ui';
import {__experimentalScrollable as Scrollable } from '@wordpress/components/ui';

function Example() {
return (
<Scrollable style={ { maxHeight: 200 } }>
<View style={ { height: 500 } }>...</View>
<div style={ { height: 500 } }>...</div>
</Scrollable>
);
}
```

## Props

##### scrollDirection
### `scrollDirection`: `string`

**Type**: `x` | `y` | `auto`
- Required: No
- Default: `y`
- Allowed values: `x`, `y`, `auto`

Renders a scrollbar for a specific axis when content overflows.

##### smoothScroll
### `smoothScroll`: `boolean`

**Type**: `boolean`
- Required: No
- Default: `false`

Enables (CSS) smooth scrolling.
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
/**
* Internal dependencies
*/
import { createComponent } from '../utils';
import { createComponent } from '../ui/utils';
import { useScrollable } from './hook';

/**
* `Scrollable` is a layout component that content in a scrollable container.
*
* @example
* ```jsx
* import { Scrollable } from `@wordpress/components/ui`;
* import { __experimentalScrollable as Scrollable } from `@wordpress/components/ui`;

Choose a reason for hiding this comment

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

docs/manifest.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @nenel1221 , I'm not sure I'm getting your comment — could you elaborate, please?


* function Example() {
* return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ import { useMemo } from '@wordpress/element';
/**
* Internal dependencies
*/
import { useContextSystem } from '../context';
import { useContextSystem } from '../ui/context';
import * as styles from './styles';

/* eslint-disable jsdoc/valid-types */
/**
* @param {import('../context').PolymorphicComponentProps<import('./types').Props, 'div'>} props
* @param {import('../ui/context').PolymorphicComponentProps<import('./types').Props, 'div'>} props
*/
/* eslint-enable jsdoc/valid-types */
export function useScrollable( props ) {
Expand Down
82 changes: 82 additions & 0 deletions packages/components/src/scrollable/stories/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/**
* External dependencies
*/
import { boolean, select } from '@storybook/addon-knobs';
/* eslint-disable-next-line no-restricted-imports */
import React from 'react';

/**
* Internal dependencies
*/
import { View } from '../../view';
import { Scrollable } from '../';

export default {
component: Scrollable,
title: 'Components (Experimental)/Scrollable',
Copy link
Member

Choose a reason for hiding this comment

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

I think it would have been useful if the story allowed us to change the scroll direction and toggle the smooth scroll option.

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 in dcaafa7

};

export const _default = () => {
const targetRef = React.useRef( null );

const onButtonClick = () => {
targetRef.current?.focus();
};

const otherProps = {
smoothScroll: boolean(
'Scrollable: smoothScroll (hint: move focus in the scrollable container)',
false
),
scrollDirection: select(
'Scrollable: scrollDirection',
{
x: 'x',
y: 'y',
auto: 'auto',
},
'y'
),
};

const containerWidth = 300;
const containerHeight = 400;

return (
<Scrollable
style={ { height: containerHeight, width: containerWidth } }
{ ...otherProps }
>
<View
style={ {
backgroundColor: '#eee',
height:
otherProps.scrollDirection === 'x'
? containerHeight
: 1000,
width:
otherProps.scrollDirection === 'y'
? containerWidth
: 1000,
position: 'relative',
} }
>
<button onClick={ onButtonClick }>
Move focus to an element out of view
</button>
<input
ref={ targetRef }
style={ {
position: 'absolute',
bottom:
otherProps.scrollDirection === 'x' ? 'initial' : 0,
right:
otherProps.scrollDirection === 'y' ? 'initial' : 0,
} }
type="text"
value="Focus me"
/>
</View>
</Scrollable>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { css } from 'emotion';
/**
* Internal dependencies
*/
import CONFIG from '../../utils/config-values';
import { CONFIG } from '../utils';

export const scrollableScrollbar = css`
@media only screen and ( min-device-width: 40em ) {
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/ui/card/body.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { useMemo } from '@wordpress/element';
* Internal dependencies
*/
import { contextConnect, useContextSystem } from '../context';
import { Scrollable } from '../scrollable';
import { Scrollable } from '../../scrollable';
import { View } from '../../view';
import * as styles from './styles';

Expand Down
18 changes: 0 additions & 18 deletions packages/components/src/ui/scrollable/stories/index.js

This file was deleted.

3 changes: 2 additions & 1 deletion packages/components/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@
"src/h-stack/**/*",
"src/v-stack/**/*",
"src/z-stack/**/*",
"src/view/**/*"
"src/view/**/*",
"src/scrollable/**/*"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: should these be sorted alphabetically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially added this line in alphabetical order, but then noticed that the list is sorted until src/visually-hidden/**/* — and then I noticed that the "sorted" part of the list represents components that are marked as production-read, while the second (non-sorted) part is for experimental components.

Probably the best person to answer about this would be @sarayourfriend , but maybe we keep this change as-is for now and if needed sort this list alphabetically in one of the future PRs?

Copy link
Contributor Author

@ciampo ciampo Jun 7, 2021

Choose a reason for hiding this comment

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

Good point! I tried to give an explanation here

Copy link
Member

Choose a reason for hiding this comment

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

Oh, that's a good point 👍 Thanks for elaborating

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, I don't think any of that is intentional. Sorting them alphabetically is fine with me. If we want to make a separation between the experimental and prod components we should add a comment explaining that and add some spacing to make it more obvious.

Though TBH I don't think it's worth the effort to maintain two separate lists. I'd rather we just opened a PR like Marco suggested to sort them once and for all.

Copy link
Member

Choose a reason for hiding this comment

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

Just wanted to stress that this is totally a minor deal. Maybe we can sort these when we touch the list the next time, but there's definitely no need to make a special PR just to sort them. It's why I prefaced my first comment with "nit: " 😉

],
"exclude": [
"src/**/*.android.js",
Expand Down