-
Notifications
You must be signed in to change notification settings - Fork 1.2k
[TextField]: Enable proper focus when clicking the stepper #8091
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
Conversation
d397754 to
8fc45f5
Compare
size-limit report 📦
|
8fc45f5 to
3863652
Compare
7d3e034 to
052dbd3
Compare
highfieldjames
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this Gina! Love the story to help tophat!
|
This makes me so happy |
|
|
||
| export const Spinner = React.forwardRef<HTMLDivElement, SpinnerProps>( | ||
| function Spinner({onChange, onClick, onMouseDown, onMouseUp}, ref) { | ||
| function Spinner({onChange, onClick, onMouseDown, onMouseUp, onBlur}, ref) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📣 The Spinner component within TextField is the 🔼 and 🔽 when the type='number' not sure why it's called Spinner 😄 🤷🏽♀️ I'm currently making a PR to rename it to Stepper 😅
highfieldjames
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for exploring this second approach! It's nice to no longer have the focus via the ref ❤️
voiid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woooo
thiagoloschi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉
gui-santos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thx for this fix :)
9aa8cca to
2e5935d
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris-icons@6.9.0 ### Minor Changes - [#8139](#8139) [`b998ca007`](b998ca0) Thanks [@leileu](https://github.com/leileu)! - Adding content minor icon for left nav in admin - [#8094](#8094) [`94988bc26`](94988bc) Thanks [@Tamas-Kisss](https://github.com/Tamas-Kisss)! - Added major and minor icon for Papercheck - [#8121](#8121) [`f74e8ffcc`](f74e8ff) Thanks [@zecarlostorre](https://github.com/zecarlostorre)! - Added EnterMajor icon ## @shopify/polaris@10.23.0 ### Minor Changes - [#8134](#8134) [`8d80691b5`](8d80691) Thanks [@mrcthms](https://github.com/mrcthms)! - Removed the focus ring from `Listbox` options ### Patch Changes - [#8093](#8093) [`60dd5a0c5`](60dd5a0) Thanks [@highfieldjames](https://github.com/highfieldjames)! - Added `borderRadius` style to `TooltipOverlay` - [#8090](#8090) [`bdcc291a4`](bdcc291) Thanks [@emmanueletti](https://github.com/emmanueletti)! - Replaced mouse up and down events on Backdrop with onClick to close Modal - [#8131](#8131) [`6096c3492`](6096c34) Thanks [@henryyi](https://github.com/henryyi)! - Fixed Navigation item secondaryActions alignment in mobile when floating actions are enabled - [#8114](#8114) [`e6aa9c801`](e6aa9c8) Thanks [@highfieldjames](https://github.com/highfieldjames)! - Dismiss index table tooltip on mouse out - [#8091](#8091) [`23ee70d13`](23ee70d) Thanks [@ginabak](https://github.com/ginabak)! - Added `onBlur` prop to numerical steppers (`Spinner` component of `TextField`) to remove multi focus issue in `TextField`. - Updated dependencies \[[`b998ca007`](b998ca0), [`94988bc26`](94988bc), [`f74e8ffcc`](f74e8ff)]: - @shopify/polaris-icons@6.9.0 ## @shopify/plugin-polaris@0.0.30 ### Patch Changes - Updated dependencies \[]: - @shopify/polaris-migrator@0.11.1 ## @shopify/polaris-migrator@0.11.1 ### Patch Changes - Updated dependencies \[[`cd150396b`](cd15039)]: - @shopify/stylelint-polaris@5.1.1 ## @shopify/stylelint-polaris@5.1.1 ### Patch Changes - [#8097](#8097) [`cd150396b`](cd15039) Thanks [@qt314](https://github.com/qt314)! - Fix incorrect unit function categorization ## polaris.shopify.com@0.30.0 ### Minor Changes - [#8110](#8110) [`5db7778e4`](5db7778) Thanks [@yurm04](https://github.com/yurm04)! - Added New badge pattern guidance for the primary nav ### Patch Changes - [#8107](#8107) [`fc30bbd32`](fc30bbd) Thanks [@Rmnlly](https://github.com/Rmnlly)! - Adding examples for truncateText and multiple secondary actions and updating props on the documentation site - Updated dependencies \[[`b998ca007`](b998ca0), [`60dd5a0c5`](60dd5a0), [`bdcc291a4`](bdcc291), [`6096c3492`](6096c34), [`94988bc26`](94988bc), [`e6aa9c801`](e6aa9c8), [`8d80691b5`](8d80691), [`f74e8ffcc`](f74e8ff), [`23ee70d13`](23ee70d)]: - @shopify/polaris-icons@6.9.0 - @shopify/polaris@10.23.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
) ### WHY are these changes introduced? (All the same issue just were multiple 😄 ) Fixes Shopify#7035 Fixes Shopify#7250 Fixes Shopify#7718 Resolves https://github.com/Shopify/inventory-states-ux/issues/654 ### 📣 Hey reviewers, there are TWO commits I'd love your eyes on! 😄 #### COMMIT 1 approach Changed the focus from the stepper to become the `input` in order to call the `onBlur` For some reason, setting the Focus via the state (`setFocus`) in the child doesn't call the `onBlur` which caused all the issues seen above. Setting the focus directly on the `input` when using the stepper calls the `onBlur`, which fixes this. #### COMMIT 2 approach Doing the commit 1 approach meant that the focus was lost on the stepper, it actually didn't have any repercussions BUT this new approach just adds an `onBlur` down to the stepper (called Spinner 😕), which also solves this problem! ### WHAT is this pull request doing? | BEFORE | AFTER| | ----- | ----- | |<img width="150" alt="Screenshot 2023-01-18 at 5 34 21 PM" src="https://user-images.githubusercontent.com/43223543/213334839-8afc6514-232b-40e5-a584-a34fee5ae9ae.png"> | <img width="300" alt="Screenshot 2023-01-18 at 5 34 06 PM" src="https://user-images.githubusercontent.com/43223543/213334815-d6c4a293-5eda-4c1a-bd14-c2625d720e7f.png"> | <details> <summary>EXPAND TO SEE VIDEO BEFORE / AFTER BEHAVIOUR 📹 </summary> ## BEFORE https://user-images.githubusercontent.com/43223543/213334719-fdd10c12-6062-4e79-a48d-121d250afe82.mp4 ## AFTER https://user-images.githubusercontent.com/43223543/213334730-50e8fafa-9205-45ba-abfe-3daf909b028b.mp4 </details> ### How to 🎩 [Most recent storybook link](https://5d559397bae39100201eedc1-rnwtxycams.chromatic.com/?path=/story/all-components-textfield--number) 🖥 [Local development instructions](https://github.com/Shopify/polaris/blob/main/README.md#local-development) 🗒 [General tophatting guidelines](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md) 📄 [Changelog guidelines](https://github.com/Shopify/polaris/blob/main/.github/CONTRIBUTING.md#changelog) <details> <summary>Copy-paste this code in <code>playground/Playground.tsx</code>:</summary> ```jsx import React, {useCallback, useState} from 'react'; import {Page, TextField} from '../src'; export function Playground() { const [value, setValue] = useState('1'); const [value1, setValue1] = useState('1'); const [value2, setValue2] = useState('1'); const handleChange = useCallback( (newValue: React.SetStateAction<string>) => setValue(newValue), [], ); const handleChange1 = useCallback( (newValue: React.SetStateAction<string>) => setValue1(newValue), [], ); const handleChange2 = useCallback( (newValue: React.SetStateAction<string>) => setValue2(newValue), [], ); return ( <Page title="Playground"> <TextField label="Quantity" type="number" value={value} onChange={handleChange} autoComplete="off" align="left" labelHidden /> <TextField label="Quantity" type="number" value={value1} onChange={handleChange1} autoComplete="off" align="left" labelHidden selectTextOnFocus /> <TextField label="Quantity" type="number" value={value2} onChange={handleChange2} autoComplete="off" align="left" labelHidden /> </Page> ); } ``` </details> ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [x] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [x] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @shopify/polaris-icons@6.9.0 ### Minor Changes - [Shopify#8139](Shopify#8139) [`b998ca007`](Shopify@b998ca0) Thanks [@leileu](https://github.com/leileu)! - Adding content minor icon for left nav in admin - [Shopify#8094](Shopify#8094) [`94988bc26`](Shopify@94988bc) Thanks [@Tamas-Kisss](https://github.com/Tamas-Kisss)! - Added major and minor icon for Papercheck - [Shopify#8121](Shopify#8121) [`f74e8ffcc`](Shopify@f74e8ff) Thanks [@zecarlostorre](https://github.com/zecarlostorre)! - Added EnterMajor icon ## @shopify/polaris@10.23.0 ### Minor Changes - [Shopify#8134](Shopify#8134) [`8d80691b5`](Shopify@8d80691) Thanks [@mrcthms](https://github.com/mrcthms)! - Removed the focus ring from `Listbox` options ### Patch Changes - [Shopify#8093](Shopify#8093) [`60dd5a0c5`](Shopify@60dd5a0) Thanks [@highfieldjames](https://github.com/highfieldjames)! - Added `borderRadius` style to `TooltipOverlay` - [Shopify#8090](Shopify#8090) [`bdcc291a4`](Shopify@bdcc291) Thanks [@emmanueletti](https://github.com/emmanueletti)! - Replaced mouse up and down events on Backdrop with onClick to close Modal - [Shopify#8131](Shopify#8131) [`6096c3492`](Shopify@6096c34) Thanks [@henryyi](https://github.com/henryyi)! - Fixed Navigation item secondaryActions alignment in mobile when floating actions are enabled - [Shopify#8114](Shopify#8114) [`e6aa9c801`](Shopify@e6aa9c8) Thanks [@highfieldjames](https://github.com/highfieldjames)! - Dismiss index table tooltip on mouse out - [Shopify#8091](Shopify#8091) [`23ee70d13`](Shopify@23ee70d) Thanks [@ginabak](https://github.com/ginabak)! - Added `onBlur` prop to numerical steppers (`Spinner` component of `TextField`) to remove multi focus issue in `TextField`. - Updated dependencies \[[`b998ca007`](Shopify@b998ca0), [`94988bc26`](Shopify@94988bc), [`f74e8ffcc`](Shopify@f74e8ff)]: - @shopify/polaris-icons@6.9.0 ## @shopify/plugin-polaris@0.0.30 ### Patch Changes - Updated dependencies \[]: - @shopify/polaris-migrator@0.11.1 ## @shopify/polaris-migrator@0.11.1 ### Patch Changes - Updated dependencies \[[`cd150396b`](Shopify@cd15039)]: - @shopify/stylelint-polaris@5.1.1 ## @shopify/stylelint-polaris@5.1.1 ### Patch Changes - [Shopify#8097](Shopify#8097) [`cd150396b`](Shopify@cd15039) Thanks [@qt314](https://github.com/qt314)! - Fix incorrect unit function categorization ## polaris.shopify.com@0.30.0 ### Minor Changes - [Shopify#8110](Shopify#8110) [`5db7778e4`](Shopify@5db7778) Thanks [@yurm04](https://github.com/yurm04)! - Added New badge pattern guidance for the primary nav ### Patch Changes - [Shopify#8107](Shopify#8107) [`fc30bbd32`](Shopify@fc30bbd) Thanks [@Rmnlly](https://github.com/Rmnlly)! - Adding examples for truncateText and multiple secondary actions and updating props on the documentation site - Updated dependencies \[[`b998ca007`](Shopify@b998ca0), [`60dd5a0c5`](Shopify@60dd5a0), [`bdcc291a4`](Shopify@bdcc291), [`6096c3492`](Shopify@6096c34), [`94988bc26`](Shopify@94988bc), [`e6aa9c801`](Shopify@e6aa9c8), [`8d80691b5`](Shopify@8d80691), [`f74e8ffcc`](Shopify@f74e8ff), [`23ee70d13`](Shopify@23ee70d)]: - @shopify/polaris-icons@6.9.0 - @shopify/polaris@10.23.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
WHY are these changes introduced?
(All the same issue just were multiple 😄 )
Fixes #7035
Fixes #7250
Fixes #7718
Resolves https://github.com/Shopify/inventory-states-ux/issues/654
📣 Hey reviewers, there are TWO commits I'd love your eyes on! 😄
COMMIT 1 approach
Changed the focus from the stepper to become the
inputin order to call theonBlurFor some reason, setting the Focus via the state (
setFocus) in the child doesn't call theonBlurwhich caused all the issues seen above. Setting the focus directly on theinputwhen using the stepper calls theonBlur, which fixes this.COMMIT 2 approach
Doing the commit 1 approach meant that the focus was lost on the stepper, it actually didn't have any repercussions BUT this new approach just adds an
onBlurdown to the stepper (called Spinner 😕), which also solves this problem!WHAT is this pull request doing?
EXPAND TO SEE VIDEO BEFORE / AFTER BEHAVIOUR 📹
BEFORE
stepper.issue.in.prod.mp4
AFTER
With.changes.mp4
How to 🎩
Most recent storybook link
🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines
Copy-paste this code in
playground/Playground.tsx:🎩 checklist
README.mdwith documentation changes