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: Add VisuallyHidden component #18022

Merged
merged 14 commits into from
Oct 24, 2019
6 changes: 6 additions & 0 deletions docs/manifest-devhub.json
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,12 @@
"markdown_source": "../packages/components/src/sandbox/README.md",
"parent": "components"
},
{
"title": "ScreenReaderText",
"slug": "screen-reader-text",
"markdown_source": "../packages/components/src/screen-reader-text/README.md",
"parent": "components"
},
{
"title": "ScrollLock",
"slug": "scroll-lock",
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 @@ -49,6 +49,7 @@ export { default as RangeControl } from './range-control';
export { default as ResizableBox } from './resizable-box';
export { default as ResponsiveWrapper } from './responsive-wrapper';
export { default as SandBox } from './sandbox';
export { default as ScreenReaderText } from './screen-reader-text';
export { default as SelectControl } from './select-control';
export { default as Snackbar } from './snackbar';
export { default as SnackbarList } from './snackbar/list';
Expand Down
9 changes: 9 additions & 0 deletions packages/components/src/screen-reader-text/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# ScreenReaderText

A component used to render text intended for a screen reader, not visible elsewhere.
Copy link
Member

Choose a reason for hiding this comment

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

You can reference WordPress docs here, maybe also copy some parts of that document:
https://make.wordpress.org/accessibility/handbook/markup/the-css-class-screen-reader-text/


### Usage

```jsx
<ScreenReaderText> Show text for screenreader. </ScreenReaderText>
Copy link
Member

Choose a reason for hiding this comment

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

In other places we add full examples:

import { Dashicon } from '@wordpress/components';

const MyDashicon = () => (
	<div>
		<Dashicon icon="admin-home" />
		<Dashicon icon="products" />
		<Dashicon icon="wordpress" />
	</div>
);

```
10 changes: 10 additions & 0 deletions packages/components/src/screen-reader-text/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

function ScreenReaderText( props ) {
return (
<div className="screen-reader-text">
Copy link
Member

Choose a reason for hiding this comment

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

You often will want to use a different tag name as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of a div? I'm open to suggestions, should it b a prop?

Copy link
Contributor

Choose a reason for hiding this comment

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

The className here doesn't follow our guidelines. I guess it was done this way to match the existing className. I think it's fine to change it for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

In another component we used isInline prop to switch between span and div to ensure HTML stays valid in all situations. I wouldn't mind starting with div and adding that prop if we find a place where we need something inline.

Copy link
Member

@gziolo gziolo Oct 21, 2019

Choose a reason for hiding this comment

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

The thing is that WP core uses screen-reader-text.

See also #18022 (comment) about VisuallyHidden proposal.

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 we have label, p and leged with the screen-reader-text class name as well.

We can iterate if you prefer to tackle it separately. See my related comment: #18022 (comment).

Copy link
Contributor

Choose a reason for hiding this comment

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

Are all these solved by nesting VisuallyHidden inline inside the tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should decide now whether we want isInline prop or a render prop. I feel they address the same issue and I don't like API duplication :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it solves the issue because the wrapping label or legend can have additional visual styles applied which you want to remove with VisuallyHidden. We would have to double-check that though. I'm making the case based on my assumptions :)

Copy link
Member Author

Choose a reason for hiding this comment

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

You can look at BaseControl for an example usage. That solution inserts <label className='screen-reader-text'>...</label>
This makes me think we would want to use the as prop. So the implementation would be <VisuallyHidden as="label">...</VisuallyHidden>

Also looks like a bug that if you pass in hideLabelFromVision prop but do not pass an id than the BaseControl.VisualLabel will be used, so would not include the class.

{ props.children }
</div>
);
}

export default ScreenReaderText;
18 changes: 18 additions & 0 deletions packages/components/src/screen-reader-text/stories/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* Internal dependencies
*/
import ScreenReaderText from '../';
import '../style.scss';

export default { title: 'ScreenReaderText', component: ScreenReaderText };

export const _default = () => (
<>
<ScreenReaderText>
This should not show.
</ScreenReaderText>
<div>
This text will always show.
</div>
</>
);
13 changes: 13 additions & 0 deletions packages/components/src/screen-reader-text/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
.screen-reader-text {
border: 0;
clip: rect(1px, 1px, 1px, 1px);
-webkit-clip-path: inset(50%);
clip-path: inset(50%);
height: 1px;
margin: -1px;
overflow: hidden;
padding: 0;
position: absolute;
width: 1px;
word-wrap: normal !important;
}
1 change: 1 addition & 0 deletions packages/components/src/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
@import "./resizable-box/style.scss";
@import "./responsive-wrapper/style.scss";
@import "./sandbox/style.scss";
@import "./screen-reader-text/style.scss";
@import "./scroll-lock/style.scss";
@import "./select-control/style.scss";
@import "./snackbar/style.scss";
Expand Down
11 changes: 10 additions & 1 deletion packages/components/storybook/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,16 @@ module.exports = ( { config } ) => {
test: /\/stories\/.+\.js$/,
loaders: [ require.resolve( '@storybook/source-loader' ) ],
enforce: 'pre',
} );
},
{
test: /\.scss$/,
loaders: [
require.resolve( 'style-loader' ),
require.resolve( 'css-loader' ),
require.resolve( 'sass-loader' ),
],
}
);

return config;
};