Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

chore: remove isFromKeyboard prop from components #1850

Merged
merged 9 commits into from
Sep 4, 2019

Conversation

layershifter
Copy link
Member

@layershifter layershifter commented Aug 26, 2019

BREAKING CHANGES

  1. Alert, Attachment, Button, ChatMessage, Checkbox, Embed, Grid, ListItem, RadioGroupItem, Reaction, Slider, ToolbarCustomItem, ToolbarItem, ToolbarMenuItem do not have isFromKeyboard in their style functions anymore. Use :focus-visible selector instead.
  2. AttachmentState, ButtonState, GridState, ListItemState, ReactionState, ToolbarCustomItemState, ToolbarItemState, ToolbarMenuItemState interfaces are no longer exported.

Before

const buttonStyles = {
  root: ({ props: p }) => ({
    ...p.isFromKeyboard && ({ border: '1px solid red' })
  })
}

After

const buttonStyles = {
  root: ({ props: p }) => ({
    ':focus-visible': { border: '1px solid red' },
  })
}

💣 Issue

To understand that event comes from a keyboard currently we have isFromKeyboard understate component's state. This solution creates at least two issues:

  • it's not scalable: to apply styles we need to update a component
  • it's not performant: to apply styles we have onFocus handlers that perform state changes and useless component render

💡 Idea

The original proposal (#749) was to use polyfill for :focus-visible, but we meet at least two issues:

  • the polyfill has some bugs
  • it's not supported really at any browser

To solve this I propose to use :focus-visible in our styles that will simplify the migration later and create a custom Fela enhancer.

Why not plugin?

Fela plugins allow to modify styles, but to make this working I should change selector generation.

🏎 What it does?

This enhancer performs a transform of generated selectors to replace :focus-visible with html[data-whatinput="keyboard"] + :focus.

buttonStyles.ts

{
  ':focus': { color: 'red' },
  ':focus-visible': { color: 'green' },
}

Generated styles

a:focus { color: red }
html[data-whatinput="keyboard"] a:focus { color: green }

The original prototype is there: https://codesandbox.io/s/young-dust-fy3b0

Untouched components

Menu & Dropdown components still will have isFromKeyboard under theirs states because they style container elements and this should be solved by :focus-within instead or other tricks separately.

@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #1850 into master will increase coverage by 0.24%.
The diff coverage is 34.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1850      +/-   ##
==========================================
+ Coverage   69.64%   69.88%   +0.24%     
==========================================
  Files         874      875       +1     
  Lines        7612     7534      -78     
  Branches     2216     2181      -35     
==========================================
- Hits         5301     5265      -36     
+ Misses       2303     2261      -42     
  Partials        8        8
Impacted Files Coverage Δ
packages/react/src/components/Button/Button.tsx 71.42% <ø> (-0.99%) ⬇️
...nts/HierarchicalTree/hierarchicalTreeItemStyles.ts 50% <ø> (ø) ⬆️
.../themes/teams/components/Chat/chatMessageStyles.ts 2.32% <ø> (ø) ⬆️
...kages/react/src/components/Toolbar/ToolbarItem.tsx 46.29% <ø> (-1.92%) ⬇️
packages/react/src/components/List/ListItem.tsx 100% <ø> (ø) ⬆️
...src/themes/teams/components/Slider/sliderStyles.ts 40% <ø> (+6.66%) ⬆️
packages/react/src/lib/felaRenderer.tsx 75% <ø> (ø) ⬆️
...react/src/components/RadioGroup/RadioGroupItem.tsx 80% <ø> (-5.72%) ⬇️
...ackages/react/src/components/Reaction/Reaction.tsx 75% <ø> (-6.82%) ⬇️
...es/teams/components/Attachment/attachmentStyles.ts 9.09% <ø> (+0.75%) ⬆️
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d7c53e...60a9980. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #1850 into master will increase coverage by 0.17%.
The diff coverage is 52.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1850      +/-   ##
==========================================
+ Coverage   69.66%   69.84%   +0.17%     
==========================================
  Files         887      888       +1     
  Lines        7813     7762      -51     
  Branches     2262     2242      -20     
==========================================
- Hits         5443     5421      -22     
+ Misses       2360     2331      -29     
  Partials       10       10
Impacted Files Coverage Δ
...rc/themes/teams/components/Tree/treeTitleStyles.ts 50% <ø> (ø) ⬆️
packages/react/src/components/Button/Button.tsx 71.42% <ø> (-0.99%) ⬇️
...nts/HierarchicalTree/hierarchicalTreeItemStyles.ts 50% <ø> (ø) ⬆️
.../themes/teams/components/Chat/chatMessageStyles.ts 2.32% <ø> (ø) ⬆️
...kages/react/src/components/Toolbar/ToolbarItem.tsx 46.29% <ø> (-1.92%) ⬇️
packages/react/src/components/List/ListItem.tsx 100% <ø> (ø) ⬆️
packages/react/src/lib/felaRenderer.tsx 75% <ø> (ø) ⬆️
...react/src/components/RadioGroup/RadioGroupItem.tsx 80% <ø> (-5.72%) ⬇️
...ackages/react/src/components/Reaction/Reaction.tsx 75% <ø> (-6.82%) ⬇️
...themes/teams/components/Checkbox/checkboxStyles.ts 5.88% <ø> (ø) ⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80e0faa...a6505ea. Read the comment docs.

@levithomason
Copy link
Member

👍

@layershifter layershifter changed the title chore: remove isFromKeyboard prop components chore: remove isFromKeyboard prop from components Aug 29, 2019
…/github.com/stardust-ui/react into feat/remove-is-from-keyboard

# Conflicts:
#	CHANGELOG.md
#	packages/react/src/components/Attachment/Attachment.tsx
#	packages/react/src/components/Button/Button.tsx
#	packages/react/src/components/Embed/Embed.tsx
#	packages/react/src/components/List/ListItem.tsx
#	packages/react/src/components/Reaction/Reaction.tsx
#	packages/react/src/components/Toolbar/ToolbarCustomItem.tsx
@vercel vercel bot temporarily deployed to staging September 3, 2019 14:33 Inactive
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants