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

Font Library: Improve usability of font variant selection #56158

Merged
merged 17 commits into from
Dec 1, 2023
Merged
Show file tree
Hide file tree
Changes from 11 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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Enhancements

- `Button`: Add focus rings to focusable disabled buttons ([#56383](https://github.com/WordPress/gutenberg/pull/56383)).
- `CheckboxControl`: Add option to not render label ([#56158](https://github.com/WordPress/gutenberg/pull/56158)).

### Bug Fix

Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/checkbox-control/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,12 @@ const MyCheckboxControl = () => {
The set of props accepted by the component will be specified below.
Props not included in this set will be applied to the input element.

#### `label`: `string`
#### `label`: `string|false`

A label for the input field, that appears at the side of the checkbox.
The prop will be rendered as content a label element.
If no prop is passed an empty label is rendered.
If the prop is set to false no label is rendered.

- Required: No

Expand Down
14 changes: 8 additions & 6 deletions packages/components/src/checkbox-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -125,12 +125,14 @@ export function CheckboxControl(
/>
) : null }
</span>
<label
className="components-checkbox-control__label"
htmlFor={ id }
>
{ label }
</label>
{ label !== false && (
mikachan marked this conversation as resolved.
Show resolved Hide resolved
<label
className="components-checkbox-control__label"
htmlFor={ id }
>
{ label }
</label>
) }
</BaseControl>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ Snapshot Diff:
>
<input
class="components-checkbox-control__input"
- id="inspector-checkbox-control-5"
+ id="inspector-checkbox-control-6"
- id="inspector-checkbox-control-6"
+ id="inspector-checkbox-control-7"
type="checkbox"
value="1"
+ />
Expand All @@ -33,8 +33,8 @@ Snapshot Diff:
</span>
<label
class="components-checkbox-control__label"
- for="inspector-checkbox-control-5"
+ for="inspector-checkbox-control-6"
- for="inspector-checkbox-control-6"
+ for="inspector-checkbox-control-7"
/>
</div>
</div>
Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/checkbox-control/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ describe( 'CheckboxControl', () => {
expect( label ).toBeInTheDocument();
} );

it( 'should not render label element if label is set to false', () => {
render( <CheckboxControl label={ false } /> );

const label = screen.queryByRole( 'label' );
expect( label ).not.toBeInTheDocument();
} );

it( 'should render a checkbox in an indeterminate state', () => {
render( <CheckboxControl indeterminate /> );
expect( getInput() ).toHaveProperty( 'indeterminate', true );
Expand Down
5 changes: 3 additions & 2 deletions packages/components/src/checkbox-control/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,10 @@ export type CheckboxControlProps = Pick<
/**
* A label for the input field, that appears at the side of the checkbox.
* The prop will be rendered as content a label element. If no prop is
* passed an empty label is rendered.
* passed an empty label is rendered. If the prop is set to false no label
* is rendered.
*/
label?: string;
label?: string | false;
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually try to avoid mixing different primitive types on the same prop, where possible.

/**
* If checked is true the checkbox will be checked. If checked is false the
* checkbox will be unchecked. If no value is passed the checkbox will be
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,13 @@
* WordPress dependencies
*/
import { CheckboxControl, Flex } from '@wordpress/components';
/**
* Internal dependencies
*/
import { getFontFaceVariantName } from './utils';

/**
* Internal dependencies
*/
import { getFontFaceVariantName } from './utils';
import FontFaceDemo from './font-demo';
import { kebabCase } from '../../../../../block-editor/src/utils/object';

function CollectionFontVariant( {
face,
Expand All @@ -27,18 +25,26 @@ function CollectionFontVariant( {
};

const displayName = font.name + ' ' + getFontFaceVariantName( face );
const checkboxId = kebabCase(
`${ font.slug }-${ getFontFaceVariantName( face ) }`
);

return (
<div className="font-library-modal__library-font-variant">
<label
className="font-library-modal__library-font-variant"
htmlFor={ checkboxId }
>
<Flex justify="space-between" align="center" gap="1rem">
<FontFaceDemo fontFace={ face } text={ displayName } />
<CheckboxControl
checked={ selected }
onChange={ handleToggleActivation }
__nextHasNoMarginBottom={ true }
id={ checkboxId }
label={ false }
/>
</Flex>
</div>
</label>
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,14 @@
*/
import { useContext } from '@wordpress/element';
import { CheckboxControl, Flex } from '@wordpress/components';
/**
* Internal dependencies
*/
import { getFontFaceVariantName } from './utils';

/**
* Internal dependencies
*/
import { getFontFaceVariantName } from './utils';
import { FontLibraryContext } from './context';
import FontFaceDemo from './font-demo';
import { kebabCase } from '../../../../../block-editor/src/utils/object';

function LibraryFontVariant( { face, font } ) {
const { isFontActivated, toggleActivateFont } =
Expand All @@ -36,18 +34,26 @@ function LibraryFontVariant( { face, font } ) {
};

const displayName = font.name + ' ' + getFontFaceVariantName( face );
const checkboxId = kebabCase(
`${ font.slug }-${ getFontFaceVariantName( face ) }`
);

return (
<div className="font-library-modal__library-font-variant">
<label
className="font-library-modal__library-font-variant"
htmlFor={ checkboxId }
>
<Flex justify="space-between" align="center" gap="1rem">
<FontFaceDemo fontFace={ face } text={ displayName } />
<CheckboxControl
checked={ isIstalled }
onChange={ handleToggleActivation }
__nextHasNoMarginBottom={ true }
id={ checkboxId }
label={ false }
/>
</Flex>
</div>
</label>
);
}

Expand Down
21 changes: 21 additions & 0 deletions test/e2e/specs/site-editor/font-library.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,26 @@ test.describe( 'Font Library', () => {
page.getByRole( 'heading', { name: 'Fonts' } )
).toBeVisible();
} );

test( 'should show font variant panel when clicking on a font family', async ( {
page,
} ) => {
await page.getByRole( 'button', { name: /styles/i } ).click();
await page
.getByRole( 'button', { name: /typography styles/i } )
.click();
await page
.getByRole( 'button', {
name: /manage fonts/i,
} )
.click();
await page.getByRole( 'button', { name: /system font/i } ).click();
await expect(
page.getByRole( 'heading', { name: /system font/i } )
).toBeVisible();
await expect(
page.getByRole( 'checkbox', { name: /system font normal/i } )
).toBeVisible();
} );
} );
} );
Loading