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

feat(theming): add focus styling utilities #1542

Merged
merged 19 commits into from
May 3, 2023
Merged

feat(theming): add focus styling utilities #1542

merged 19 commits into from
May 3, 2023

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented May 1, 2023

Description

This PR adds xs units to the DEFAULT_THEME to account for updated "spacer" focus styling, along with three new utilities:

  • focusStyles (preferred) provides a CSS block with focus outline reset, focus-visible box-shadow styling, and transparent outline hack for high contrast mode
  • getFocusBoxShadow (fallback) in the case where existing code would be a tedious/surgical refactor, this utility provides the CSS value for the box-shadow property
  • SELECTOR_FOCUS_VISIBLE constant that can be used together with the selector parameter on focusStyles:
focusStyles({ theme: props.theme, selector: `&:focus-within, ${SELECTOR_FOCUS_VISIBLE}` });

Detail

For performance concerns, the getColor utility was memoized, resulting in an extensive diff due to the memoize function wrap.

This PR contains two commits (rolled back), demonstrating real utility usage within a couple of key styled components:

  • StyledTextInput using focusStyles (note that the existing :focus block should've also been removed)
  • StyledButton using getFocusBoxShadow (focusStyles could be applied with extra effort)

Checklist

  • πŸ‘Œ design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🀘 renders as expected with Bedrock CSS (?bedrock)
  • πŸ’‚β€β™‚οΈ includes new unit tests. Maintain existing coverage (always >= 96%)
  • β™Ώ tested for WCAG 2.1 AA accessibility compliance
  • πŸ“ tested in Chrome, Firefox, Safari, and Edge

@jzempel jzempel requested a review from a team as a code owner May 1, 2023 11:59
@coveralls
Copy link

coveralls commented May 1, 2023

Coverage Status

Coverage: 96.146% (-0.003%) from 96.149% when pulling fcdfd95 on jzempel/focus into 4d7f950 on main.


const palette = theme && theme.palette ? theme.palette : DEFAULT_THEME.palette;
const colors = theme && theme.colors ? theme.colors : DEFAULT_THEME.colors;
export const getColor = memoize(
Copy link
Contributor

Choose a reason for hiding this comment

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

Were there any performance concerns that prompted the lodash.memoize utility usage?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, calling out to these utilities results in repeat getColor calls in a lot of existing cases. But I think there's a massive savings potential overall.

* ```
*/
export const focusStyles = ({
condition = true,
Copy link
Contributor

@Francois-Esquire Francois-Esquire May 1, 2023

Choose a reason for hiding this comment

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

I'm curious, what advantage does the condition argument bring? The condition argument can be done in the context of the code where focusStyles is used; which reads clearer and less surface area to debug.

Copy link
Member Author

Choose a reason for hiding this comment

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

See commit linked in description


/*
* 1. Browser reset
* 2. High contrast mode hack
Copy link
Contributor

@Francois-Esquire Francois-Esquire May 1, 2023

Choose a reason for hiding this comment

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

  • Can you please share your resource/research on the high contrast mode hack? Very curious to learn more about this approach.
  • Will there need to be other parts to this mechanism? An extension to the theme colors with a "high contrast color" token(s)?

Copy link
Member Author

Choose a reason for hiding this comment

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

See link in description

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for transparent.

Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

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

Nothing major to comment on. I dig it.

* @param {string} [options.shadowWidth='md'] Provides a theme object `shadowWidth` key for the cumulative width of the `box-shadow`
* @param {string} [options.spacerWidth='xs'] Provides a theme object `shadowWidth` for the white spacer
* @param {Object} [options.styles] Adds CSS property values to be rendered with `:focus-visible`
* @param {Object} options.theme Provides values used to resolve the desired color
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @param {Object} options.theme Provides values used to resolve the desired color
* @param {Object} [options.theme] Provides values used to resolve the desired color

Assuming the brackets were left out on accident. :)

packages/theming/src/utils/getColor.ts Show resolved Hide resolved
import { math } from 'polished';
import { FocusBoxShadowParameters, getFocusBoxShadow } from './getFocusBoxShadow';

export const SELECTOR_FOCUS_VISIBLE = '&:focus-visible, &[data-garden-focus-visible="true"]';
Copy link
Contributor

Choose a reason for hiding this comment

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

:focus-visible πŸš€ πŸš€

Copy link
Contributor

@Francois-Esquire Francois-Esquire left a comment

Choose a reason for hiding this comment

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

Looks sharp!

@jzempel jzempel merged commit 49e88f3 into main May 3, 2023
1 check passed
@jzempel jzempel deleted the jzempel/focus branch May 3, 2023 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

5 participants