[TextField] Add ability to hide input type="number" stepper arrows by default until the field is interacted with

[tatianau]

WHY are these changes introduced?

The feature is highly requested by the admin teams in layouts when more than one field is used which makes the layouts visually busy when all steppers are shown. Not showing the steppers until interacted is also the default browser behaviour.

Most recent requests:

• products team
• catalogs team
• inventory team

Resolves https://github.com/Shopify/core-issues/issues/52378

WHAT is this pull request doing?

This PR is adding support for hiding the stepper arrows if the input type is set to number and showing when the field is interacted with (hover, focus) to mimic the default browser behaviour.

Before

After

09-54-40fwk-oxmyb.mp4

How to 🎩

Any of the tophatting options produce the same result

Admin test page

• Open the spin URL

Storybook playground

• Checkout this branch
• Run yarn turbo run dev --filter=@shopify/polaris to open storybook
• Open the TextField component's Number variation
• Can also copy the details below into your polaris-react/playground/Playground.tsx to see how it works in different contexts

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

import React, {useCallback, useState} from 'react';

import {
  AlphaCard,
  Columns,
  Page,
  TextField,
  Text,
  Box,
  Inline,
  AlphaStack,
} from '../src';

export function Playground() {
  const [value, setValue] = useState('1');
  const handleChange = useCallback((newValue) => setValue(newValue), []);

  return (
    <Page title="Playground">
      <AlphaStack gap="5">
        <AlphaCard>
          <Text as="h2" variant="headingMd">
            Controlled width example with 1fr `Columns` component
          </Text>
          <Box paddingBlockStart="4">
            <Columns
              columns={{
                md: '1fr 1fr',
              }}
              gap={{
                md: '2',
              }}
            >
              <TextField
                label="Quantity"
                type="number"
                value={value}
                onChange={handleChange}
                autoComplete="off"
              />
              <TextField
                label="Minimum"
                type="number"
                value={value}
                onChange={handleChange}
                autoComplete="off"
              />
            </Columns>
          </Box>
        </AlphaCard>
        <AlphaCard>
          <Text as="h2" variant="headingMd">
            Uncontrolled width example with `Inline` component
          </Text>
          <Box paddingBlockStart="4">
            <Inline gap="4">
              <TextField
                label="Quantity"
                type="number"
                value={value}
                onChange={handleChange}
                autoComplete="off"
              />
              <TextField
                label="Minimum"
                type="number"
                value={value}
                onChange={handleChange}
                autoComplete="off"
              />
            </Inline>
          </Box>
        </AlphaCard>
      </AlphaStack>
    </Page>
  );
}

🎩 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

[github-actions]

size-limit report 📦

Path	Size
polaris-react-cjs	221.31 KB (0%)
polaris-react-esm	140.76 KB (0%)
polaris-react-esnext	197.45 KB (+0.02% 🔺)
polaris-react-css	43.04 KB (+0.07% 🔺)

[alex-page]

Can this be the default behaviour? I prefer to ship strong design opinions instead of adding properties that multiply the number of ways the component looks.

[laurkim]

Having it be the default behavior also mitigates conflicting UIs in different sections in the admin where some TextFields wouldn't have this prop enabled

[tatianau]

@alex-page @laurkim Thank you for your suggestions!

I can see how we may enforce it eventually as a strong design opinion in the future as it makes sense for the component to mimic the default browser behaviour by default. However, I'm not sure if we can do it right away without introducing breaking changes for consumers.

Conditionally hiding and showing the spinner may result in an unexpected layout jump caused by increased textfield width depending on how consumers handle those text field layouts at the parent level.

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

import React, {useCallback, useState} from 'react';

import {
  AlphaCard,
  Columns,
  Page,
  TextField,
  Text,
  Box,
  Inline,
  AlphaStack,
} from '../src';

export function Playground() {
  const [value, setValue] = useState('1');
  const handleChange = useCallback((newValue) => setValue(newValue), []);

  return (
    <Page title="Playground">
      <AlphaStack gap="5">
        <AlphaCard>
          <Text as="h2" variant="headingMd">
            Controlled width example with 1fr `Columns` component
          </Text>
          <Box paddingBlockStart="4">
            <Columns
              columns={{
                md: '1fr 1fr',
              }}
              gap={{
                md: '2',
              }}
            >
              <TextField
                label="Quantity"
                type="number"
                value={value}
                onChange={handleChange}
                autoComplete="off"
                stepperShownOnInteraction
              />
              <TextField
                label="Minimum"
                type="number"
                value={value}
                onChange={handleChange}
                autoComplete="off"
                stepperShownOnInteraction
              />
            </Columns>
          </Box>
        </AlphaCard>
        <AlphaCard>
          <Text as="h2" variant="headingMd">
            Uncontrolled width example with `Inline` component
          </Text>
          <Box paddingBlockStart="4">
            <Inline gap="4">
              <TextField
                label="Quantity"
                type="number"
                value={value}
                onChange={handleChange}
                autoComplete="off"
                stepperShownOnInteraction
              />
              <TextField
                label="Minimum"
                type="number"
                value={value}
                onChange={handleChange}
                autoComplete="off"
                stepperShownOnInteraction
              />
            </Inline>
          </Box>
        </AlphaCard>
      </AlphaStack>
    </Page>
  );
}

09-16-3h7k2-iqpgp.mp4

---

@alex-page Would we consider this change as an interim step towards global change while giving the teams a warning that soon it will become the default experience and they need to use this prop proactively in their components and adjust the layouts if needed before the next major version with breaking changes rolls out deprecating this prop as a configurable option?

[alex-page]

Breaking changes are good. We definitely should break more stuff.

[voiid]

@zecarlostorre did mention it was something we were considering as an evolution of the design language. I do think it would be a nice change given how busy a lot of these surfaces get so I'm certainly for it. Any steps needed for alignment?

[tatianau]

Would the following work?

• v10.36.0
  • Ship this PR with the ability to configure the stepper via a prop
  • Inform the teams that they should add the prop to all input=“number” components and update the layouts if broken to give them time to migrate and adjust properly considering their project commitments so that there are no frustrations from teams and merchants using those UIs
• v11.0.0
  • Remove this prop to enable the behaviour by default - by the time this ships, the majority of the teams will have adjusted to this change so it doesn’t come as a surprise

Or what option would be ideal in this case @alex-page?

[alex-page]

I don't think we need major alignment here. It's the best solution for the long term and default design. We should be confident making those changes.

The Inline bug above should be addressed. We don't want the input jumping like that in 1P/3P apps if they update their Polaris version.

I could see this shipping without it being a breaking change and just applying the styles to the input only. I might be missing the reason why this needs to be a breaking change. Adding a property to remove it in the next version is super confusing to developers, educating them to use something and then it being removed is probably not the best approach.

[tatianau]

The <Inline> is not a bug, it’s just an example of an uncontrolled children’s width from a parent level. It could happen the same way if <Columns> are used with something more flexible than controlled width (e.g. auto or minmax() instead of fr or percentage) and there are a lot of other cases that don’t explicitly control width that could break the same way.

All that to say, 

I’m all for boldly shipping this and for strong design opinions, just wondering who’d be handling the aftermath in case it breaks for consumers but maybe I'm overthinking it.

If we are confident it won't break - I can update the PR to enforce this behaviour by default and remove the prop. I'' update the spin instance in the admin and that to see if there is something immediately obvious that's broken or not.

[alex-page]

Is this an issue that exists today or only exists because of this change? If it is only because of this change then we should look into addressing why the HTML is changing and causing layout shift on hover.

[tatianau]

NVM 🙈 All fixed now.

[alex-page]

Is there a reason we can't use CSS :hover?

[tatianau]

The main reason is the separation of concerns. The existing rendering logic for the spinner is all handled in the component with conditional rendering. If we were to handle the rendering of the stepper on hover in the stylesheet - this would result in the fragmentation of those rendering responsibilities when some are in the component, and others in the styles, and it may be less clear if they are coupled or not, e.g. the component logic may be saying I'm rendered while the stylesheet is no I'm actually not unless hovered over. So I thought it would make sense to follow the existing pattern without splitting those responsibilities. Let me know if you think otherwise.

[alex-page]

We definitely want to avoid adding event code when it mirrors what we can do in CSS. I understand why you did this but I think it should be a CSS change.

[tatianau]

Thanks, Alex, turns out I was overthinking it, have been in javascript land for too long, and the solution is actually much shorter and simpler than before 🥲 and it no longer causes the layout shift no matter what consumers do, so it should be all good.

[alex-page]

Wuhuu! It happens to everyone. Thanks @tatianau 🎉

[alex-page]

Love the turn around here @tatianau. Thanks for addressing the feedback 🙌

[laurkim]

Nice! 💯 ⭐

[stefanlegg]

Love it, nice job!