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

Jump to bottom

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 4 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
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 @@ -5,6 +5,7 @@
### Bug Fix

- `Popover`: Pin `react-dropdown-menu` version to avoid breaking changes in dependency updates. ([52356](https://github.com/WordPress/gutenberg/pull/52356)).
- `ItemGroup`: Apply focus style to `Item` component rendered as anchor element. ([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
55 changes: 29 additions & 26 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 @@ -8,32 +8,35 @@ import { css } from '@emotion/react';
*/
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;
}
`;
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`
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-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;
}
`;
};

export const itemWrapper = css`
width: 100%;
Expand Down
6 changes: 3 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 @@ -618,9 +618,9 @@ exports[`EditPostPreferencesModal should match snapshot when the modal is active

.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;
}

Expand Down
11 changes: 0 additions & 11 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 @@ -22,17 +22,6 @@
fill: $gray-700;
}

&: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.

}

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