Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

Implement focus visible provider #659

Merged
merged 1 commit into from
Apr 27, 2020

Conversation

nickoola
Copy link
Contributor

No description provided.

@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch 2 times, most recently from 7a1bc0e to 037989c Compare April 10, 2020 13:42
@nickoola
Copy link
Contributor Author

nickoola commented Apr 10, 2020

Interesting discussion about context re-render:
facebook/react#15156

And cool article to check re-render with context:
https://frontarm.com/james-k-nelson/react-context-performance/

@nickoola nickoola requested a review from wakooka April 10, 2020 14:01
@simonrelet
Copy link
Contributor

I'm having difficulties seeing how this will be used.

For example the Button component is structured as following:

// Button.tsx
class Button extends PureComponent<P, S> {
  // ...
}

// index.tsx
const StyledButton = styled(Button)`
  /* [...] */
`

So in order to use the hook we need to transform the Button to a functional component:

// Button.tsx
function Button({ className }) {
  const { focusVisible, onFocus, onBlur } = useFocusVisible()
  // ...

  // Simplified for the example
  return (
    <button
      onFocus={onFocus}
      onBlur={onBlur}
      className={cc([className, prefix({ focusVisible })])}
    />
  )
}

// index.tsx
const StyledButton = styled(Button)`
  /* ... */

  &.kirk-focusVisible {
    /* ... */
  }
`

Is that right?

@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch from 523d68f to dc1efe7 Compare April 14, 2020 17:40
}

export const FocusVisibleContext = createContext({
hadKeyboardEvent: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setted to false as initial value cause not the common behavior.

src/why/index.tsx Outdated Show resolved Hide resolved
@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch 3 times, most recently from 1187e97 to 498b6a4 Compare April 15, 2020 10:55
@nickoola
Copy link
Contributor Author

nickoola commented Apr 15, 2020

I'm having difficulties seeing how this will be used.

For example the Button component is structured as following:

// Button.tsx
class Button extends PureComponent<P, S> {
  // ...
}

// index.tsx
const StyledButton = styled(Button)`
  /* [...] */
`

So in order to use the hook we need to transform the Button to a functional component:

// Button.tsx
function Button({ className }) {
  const { focusVisible, onFocus, onBlur } = useFocusVisible()
  // ...

  // Simplified for the example
  return (
    <button
      onFocus={onFocus}
      onBlur={onBlur}
      className={cc([className, prefix({ focusVisible })])}
    />
  )
}

// index.tsx
const StyledButton = styled(Button)`
  /* ... */

  &.kirk-focusVisible {
    /* ... */
  }
`

Is that right?

Yes it'll be used this way + having the FocusVisibleProvider as wrapper like we do with MediaSizeProvider

@nickoola nickoola changed the title [Work in progress] Implement focus visible provider Implement focus visible provider Apr 15, 2020
@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch from 047426a to 764552c Compare April 15, 2020 14:25
src/_utils/focusVisible/FocusVisibleProvider.tsx Outdated Show resolved Hide resolved
src/_utils/focusVisible/FocusVisibleProvider.tsx Outdated Show resolved Hide resolved
src/_utils/focusVisible/FocusVisibleProvider.tsx Outdated Show resolved Hide resolved
src/_utils/focusVisible/FocusVisibleProvider.tsx Outdated Show resolved Hide resolved
@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch from 764552c to b1c1532 Compare April 17, 2020 12:17
@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch 5 times, most recently from ca5e5cf to 4851f21 Compare April 21, 2020 17:01

export const FocusVisibleContext = createContext(false)

export const FocusVisibleProvider = (props: PropsWithChildren<{}>) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonrelet I use PropsWithChildren as type for children instead of interface like:

interface FocusVisibleProviderProps {
  children: ReactNode
}

Not typescript expert so just want to know if it's the right way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have rarely seen PropsWithChildren before, but I don't feel comfortable having a component with props but no exported prop types.

If we really want to avoid the children?: React.ReactNode part maybe we can write:

export type FocusVisibleProviderProps = PropsWithChildren<{}>

But the empty type (<{}>) still feels weird.

Having the extended type declaration (children?: React.ReactNode) wouldn't bother me much.

Not sure my comment helps here 😅.

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 don't really have strong opinion for this point so I'll fallback with:

type FocusVisibleProviderProps = {
  children: ReactNode
}

@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch from 7ca450c to 65c1608 Compare April 21, 2020 17:44
src/_utils/keycodes.ts Outdated Show resolved Hide resolved
src/_utils/focusVisible/index.tsx Outdated Show resolved Hide resolved
@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch from 1aa8730 to 3b42dc0 Compare April 22, 2020 15:20
@nickoola nickoola requested a review from wakooka April 22, 2020 15:23
@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch 2 times, most recently from e700f37 to ea2481b Compare April 23, 2020 10:12
@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch 2 times, most recently from 7dbe8a5 to 08bbbe8 Compare April 23, 2020 12:36
Copy link
Contributor

@simonrelet simonrelet left a comment

Choose a reason for hiding this comment

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

Nice work 👍

@gabrielagabriel
Copy link
Contributor

Could you "illustrate" this PR? Thanks

Copy link
Contributor

@gabrielagabriel gabrielagabriel left a comment

Choose a reason for hiding this comment

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

Nice refactor! Great improvement on code readability 🦉

@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch from 08bbbe8 to 1188f7c Compare April 27, 2020 12:35
@nickoola nickoola force-pushed the BBC-7512-implement-focus-visible-polyfill branch from 1188f7c to 65855ff Compare April 27, 2020 12:37
@nickoola nickoola merged commit e519429 into master Apr 27, 2020
@nickoola nickoola deleted the BBC-7512-implement-focus-visible-polyfill branch April 27, 2020 13:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants