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

Item: Unify focus style and add default font styles #52495

Merged
merged 10 commits into from
Jul 13, 2023
Merged

Item: Unify focus style and add default font styles #52495

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
48a9691
ItemGroup: Apply focus state to `Item` component rendered as anchor e…
t-hamano Jul 11, 2023
ebe07be
Update changelog
t-hamano Jul 11, 2023
821a377
Don't apply text-decoration in the case of a button element
t-hamano Jul 11, 2023
1194265
Update snapshot
t-hamano Jul 11, 2023
5e88060
Merge branch 'trunk' into item/fix-focus-state-as-a
t-hamano Jul 12, 2023
48b6050
Update storybook and add default styles to anchor element
t-hamano Jul 12, 2023
672e399
Update snapshot
t-hamano Jul 12, 2023
74cdb6b
Update changelog
t-hamano Jul 12, 2023
9e29aab
Resolve conflicts
t-hamano Jul 13, 2023
51ef2f8
Fix incorrect changelog entries
t-hamano Jul 13, 2023
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
2 changes: 1 addition & 1 deletion packages/block-library/src/block/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"title": "Pattern",
"category": "reusable",
"description": "Create and save content to reuse across your site. Update the pattern, and the changes apply everywhere it’s used.",
"keywords": [ "reusable" ],
"keywords": [ "reusable" ],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I merged the trunk, it included an unintended modification, but the indentation should be correct with this change 🙈

"textdomain": "default",
"attributes": {
"ref": {
Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
### Bug Fix

- `Popover`: Pin `react-dropdown-menu` version to avoid breaking changes in dependency updates. ([52356](https://github.com/WordPress/gutenberg/pull/52356)).
- `Item`: Unify focus style and add default font styles. ([52495](https://github.com/WordPress/gutenberg/pull/52495)).
Copy link
Member

Choose a reason for hiding this comment

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

Not very important because it's just an optional check, but FYI the CI check for the changelog is failing because this is missing a number symbol:

Suggested change
- `Item`: Unify focus style and add default font styles. ([52495](https://github.com/WordPress/gutenberg/pull/52495)).
- `Item`: Unify focus style and add default font styles. ([#52495](https://github.com/WordPress/gutenberg/pull/52495)).


## 25.3.0 (2023-07-05)

Expand Down
3 changes: 2 additions & 1 deletion packages/components/src/item-group/item/hook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,8 @@ export function useItem( props: WordPressComponentProps< ItemProps, 'div' > ) {
const classes = useMemo(
() =>
cx(
as === 'button' && styles.unstyledButton,
( as === 'button' || as === 'a' ) &&
styles.unstyledButton( as ),
styles.itemSizes[ size ] || styles.itemSizes.medium,
styles.item,
spacedAround && styles.spacedAround,
Expand Down
11 changes: 8 additions & 3 deletions packages/components/src/item-group/stories/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,20 +39,25 @@ Default.args = {
children: (
[
{
children: 'First item',
children: 'First button item',
// eslint-disable-next-line no-alert
onClick: () => alert( 'First item clicked' ),
},
{
children: 'Second item',
children: 'Second button item',
// eslint-disable-next-line no-alert
onClick: () => alert( 'Second item clicked' ),
},
{
children: 'Third item',
children: 'Third button item',
// eslint-disable-next-line no-alert
onClick: () => alert( 'Third item clicked' ),
},
{
children: 'Anchor item',
as: 'a',
href: 'https://wordpress.org',
},
Comment on lines +56 to +60
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 added an anchor element to the Storybook, is that ok?

] as ItemProps[]
).map( mapPropsToItem ),
};
Expand Down
67 changes: 39 additions & 28 deletions packages/components/src/item-group/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,34 +6,45 @@ import { css } from '@emotion/react';
/**
* Internal dependencies
*/
import { CONFIG, COLORS } from '../utils';

export const unstyledButton = css`
appearance: none;
border: 1px solid transparent;
cursor: pointer;
background: none;
text-align: start;

svg,
path {
fill: currentColor;
}

&:hover {
color: ${ COLORS.ui.theme };
}

&:focus-visible {
box-shadow: 0 0 0 var( --wp-admin-border-width-focus )
var(
--wp-components-color-accent,
var( --wp-admin-theme-color, ${ COLORS.ui.theme } )
);
// Windows high contrast mode.
outline: 2px solid transparent;
}
`;
import { CONFIG, COLORS, font } from '../utils';

export const unstyledButton = ( as: 'a' | 'button' ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should we also add an explicit font-size to normalize the sizes between button and anchor? (Like this)

Different font sizes in Storybook

In Storybook we can see the font sizes will be different due to the user agent styles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see: #52495 (comment)

return css`
font-size: ${ font( 'default.fontSize' ) };
font-family: inherit;
Comment on lines +13 to +14
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we also add an explicit font-size to normalize the sizes between button and anchor? (Like this)

I noticed that in addition to font size, the browser default font family was applied to the button element, so I added an inherited style.

appearance: none;
border: 1px solid transparent;
cursor: pointer;
background: none;
text-align: start;
text-decoration: ${ as === 'a' ? 'none' : undefined };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add a style that cancels the default underline only for the anchor element.


svg,
path {
fill: currentColor;
}

&:hover {
color: ${ COLORS.ui.theme };
}

&:focus {
box-shadow: none;
outline: none;
}

&:focus-visible {
box-shadow: 0 0 0 var( --wp-admin-border-width-focus )
var(
--wp-components-color-accent,
var( --wp-admin-theme-color, ${ COLORS.ui.theme } )
);
// Windows high contrast mode.
outline: 2px solid transparent;
outline-offset: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The outline-offset was added in this PR and is needed to make the outline size consistent in Windows high contrast mode.

}
`;
};

export const itemWrapper = css`
width: 100%;
Expand Down
14 changes: 11 additions & 3 deletions packages/edit-post/src/components/preferences-modal/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,8 @@ exports[`EditPostPreferencesModal should match snapshot when the modal is active
}

.emotion-13 {
font-size: 13px;
font-family: inherit;
-webkit-appearance: none;
-moz-appearance: none;
-ms-appearance: none;
Expand All @@ -616,12 +618,18 @@ exports[`EditPostPreferencesModal should match snapshot when the modal is active
color: var(--wp-components-color-accent, var(--wp-admin-theme-color, #3858e9));
}

.emotion-13:focus {
box-shadow: none;
outline: none;
}

.emotion-13:focus-visible {
box-shadow: 0 0 0 var( --wp-admin-border-width-focus ) var(
--wp-components-color-accent,
var( --wp-admin-theme-color, var(--wp-components-color-accent, var(--wp-admin-theme-color, #3858e9)) )
);
--wp-components-color-accent,
var( --wp-admin-theme-color, var(--wp-components-color-accent, var(--wp-admin-theme-color, #3858e9)) )
);
outline: 2px solid transparent;
outline-offset: 0;
}

.emotion-15 {
Expand Down
16 changes: 0 additions & 16 deletions packages/edit-site/src/components/sidebar-navigation-item/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,22 +26,6 @@
fill: $gray-600;
}

&:is(a) {
text-decoration: none;
display: flex;
align-items: center;

&:focus {
box-shadow: none;
outline: none;
}
Comment on lines -34 to -37
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we still need to keep this reset, because the :focus state gets triggered (without being overridden by a :focus-visible state) when you click on the link:

The anchor item gets a light blue box-shadow when clicked

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This focus style seems to be derived from WP-Admin's common.css.

common

I think it would be better to resolve this at the component level, so I will move this focus style to the component.


&:focus-visible {
box-shadow: 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
outline: 2px solid transparent;
}
}

&.with-suffix {
padding-right: $grid-unit-20;
}
Expand Down