[TextField] Added new prop that disables spinner #3477

ehasrouni · 2020-10-13T20:42:52Z

WHY are these changes introduced?

Fixes https://github.com/Shopify/web/pull/32659 ; In Admin and Pos Channel, we want to hide the spinner when entering a Pin (4 digit number).

WHAT is this pull request doing?

This PR doesn't display/hides the spinner if the input type is 'number' and if the step="0".
Before (top field) & After (bottom field)

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

In the readme.md, I copy pasted the code for "Number field", but added the step="0" prop to hide the spinner. ``` ### Number field Without Spinner

Use when input text should be a pin.


function NumberFieldExample() {
  const [value, setValue] = useState('1');

  const handleChange = useCallback((newValue) => setValue(newValue), []);

  return (
    <TextField
      label="Quantity"
      type="number"
      step="0"
      value={value}
      onChange={handleChange}
    />
  );
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide
For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

ghost · 2020-10-13T20:42:57Z

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

github-actions · 2020-10-13T20:45:10Z

🟡 This pull request modifies 3 files and might impact 9 other files. This is an average splash zone for a change, remember to tophat areas that could be affected.

Details:

All files potentially affected (total: 9)

📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/TextField/TextField.tsx (total: 9)

Files potentially affected (total: 9)

🧩 src/components/TextField/tests/TextField.test.tsx (total: 0)

Files potentially affected (total: 0)

kyledurand · 2020-10-15T12:49:13Z

src/components/TextField/TextField.tsx

  | 'text'
  | 'decimal'
  | 'numeric'
+  | 'numeric-no-spinner'


This isn't a valid input type, unless it's super new and I don't know about it. I'm worried it will break accessibility. Can we add a prop to hide the spinner instead?

Hey Kyle! Initially, I thought about doing a prop to hide the spinner, but was told that perhaps a new type would be better? https://shopify.slack.com/archives/CQK6FNLJK/p1602612476356400 I might've misunderstood what Ben meant, thank you :)

dfmcphee · 2020-10-15T13:46:20Z

An alternative approach to this change is to use [0-9]* in the pattern attribute already available on TextField to tell it to only accept numbers. This will display the correct keyboard on mobile without displaying the spinner.

Example: https://codesandbox.io/s/gracious-fire-jkckq?file=/App.js

ehasrouni · 2020-10-15T14:10:02Z

An alternative approach to this change is to use [0-9]* in the pattern attribute already available on TextField to tell it to only accept numbers. This will display the correct keyboard on mobile without displaying the spinner.

Example: https://codesandbox.io/s/gracious-fire-jkckq?file=/App.js

Thank you! I just tried it and it seems to work (partially) when the input type isn't set; without setting the input type to number, it's possible to enter letters in the field, as soon as I set the input type to number, the spinner is back

dfmcphee · 2020-10-15T14:25:42Z

Ah, ok. I think in that case you would either have to go with this change but still pass a valid number type to the underlying input.

One more approach that I think I actually prefer is that if you set the step prop to 0 it hides the spinner since the spinner won't actually do anything when set to 0 anyway.

ehasrouni · 2020-10-16T16:56:25Z

Ah, ok. I think in that case you would either have to go with this change but still pass a valid number type to the underlying input.

One more approach that I think I actually prefer is that if you set the step prop to 0 it hides the spinner since the spinner won't actually do anything when set to 0 anyway.

Updated the PR with the changes suggested :)

dfmcphee

Just one suggestion for the change log but otherwise this works great. Would be good to add a test for this as well.

dfmcphee · 2020-10-19T17:34:34Z

UNRELEASED.md


 ### Enhancements

+- Added `step='0'` check to `Textfield` to disable spinner with input type 'number' ([#3477](https://github.com/Shopify/polaris-react/pull/3477))


Suggested change

- Added `step='0'` check to `Textfield` to disable spinner with input type 'number' ([#3477](https://github.com/Shopify/polaris-react/pull/3477))

- Updated `Textfield` with a `type` of `number` to not render a spinner if step is set to `0` ([#3477](https://github.com/Shopify/polaris-react/pull/3477))

dfmcphee

Nice. Looks good! Thanks for adding that.

kyledurand

Love this solution. Thanks @ehasrouni!

ghost · 2020-10-20T13:49:56Z

🎉 Thanks for your contribution to Polaris React!

* Added new inputMode type that disables spinner * Updated unreleased.md * Fixed failing job run * Updated unreleased.md * Added "step=0" to disable spinner, reverted old changes * Changed type from string to int * Added a test following PR comment * Fixed a linting issue, added onChange to test

ehasrouni requested a review from a team October 13, 2020 21:03

ehasrouni added 3 commits October 14, 2020 14:00

Added new inputMode type that disables spinner

a211f60

Updated unreleased.md

1c46b7f

Fixed failing job run

d82a6c5

ehasrouni force-pushed the add-new-textfield-number-type branch from 65dd88a to d82a6c5 Compare October 14, 2020 18:04

Updated unreleased.md

3c86a3a

kyledurand reviewed Oct 15, 2020

View reviewed changes

Added "step=0" to disable spinner, reverted old changes

2b033c2

ehasrouni changed the title ~~[TextField] Added new inputMode type that disables spinner~~ [TextField] Added new prop that disables spinner Oct 15, 2020

Changed type from string to int

86aa5ef

ehasrouni requested a review from kyledurand October 15, 2020 18:36

dfmcphee reviewed Oct 19, 2020

View reviewed changes

Added a test following PR comment

4bfde88

ehasrouni force-pushed the add-new-textfield-number-type branch from d398f94 to 4bfde88 Compare October 19, 2020 20:53

ehasrouni requested a review from dfmcphee October 19, 2020 20:54

dfmcphee approved these changes Oct 19, 2020

View reviewed changes

kyledurand approved these changes Oct 19, 2020

View reviewed changes

ehasrouni added 2 commits October 20, 2020 08:34

Fixed a linting issue, added onChange to test

b4adaec

Merge branch 'master' into add-new-textfield-number-type

7a69da1

ehasrouni merged commit e290a8c into master Oct 20, 2020

ehasrouni deleted the add-new-textfield-number-type branch October 20, 2020 13:49

ehasrouni restored the add-new-textfield-number-type branch October 20, 2020 18:19

ehasrouni deleted the add-new-textfield-number-type branch October 20, 2020 18:19


		### Enhancements

		- Added `step='0'` check to `Textfield` to disable spinner with input type 'number' ([#3477](https://github.com/Shopify/polaris-react/pull/3477))

	- Added `step='0'` check to `Textfield` to disable spinner with input type 'number' ([#3477](https://github.com/Shopify/polaris-react/pull/3477))
	- Updated `Textfield` with a `type` of `number` to not render a spinner if step is set to `0` ([#3477](https://github.com/Shopify/polaris-react/pull/3477))

[TextField] Added new prop that disables spinner #3477

[TextField] Added new prop that disables spinner #3477

Uh oh!

Conversation

ehasrouni commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

Uh oh!

ghost commented Oct 13, 2020

Uh oh!

github-actions bot commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Files potentially affected (total: 0)

Files potentially affected (total: 9)

Files potentially affected (total: 0)

Uh oh!

kyledurand Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

ehasrouni Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

dfmcphee commented Oct 15, 2020

Uh oh!

ehasrouni commented Oct 15, 2020

Uh oh!

dfmcphee commented Oct 15, 2020

Uh oh!

ehasrouni commented Oct 16, 2020

Uh oh!

dfmcphee left a comment

Choose a reason for hiding this comment

Uh oh!

dfmcphee Oct 19, 2020

Choose a reason for hiding this comment

Uh oh!

dfmcphee left a comment

Choose a reason for hiding this comment

Uh oh!

kyledurand left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Oct 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ehasrouni commented Oct 13, 2020 •

edited

Loading

github-actions bot commented Oct 13, 2020 •

edited

Loading