Skip to content
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

[TextField] Allow click event on spinner (stepper in number type) #6719

Merged
merged 2 commits into from
Jul 27, 2022

Conversation

ginabak
Copy link
Contributor

@ginabak ginabak commented Jul 21, 2022

co-author: @weslleyaraujo

WHY are these changes introduced?

Fixes #6725
Part of https://github.com/Shopify/inventory-states-ux/issues/524
Follow up to #6129 https://github.com/Shopify/web/pull/68974

As of Polaris 9.12.0, we no longer bubble up click events emitted on TextField and parent elements can no longer depend on interactions. This change was introduced here.

Click events do not bubble for the Spinner(see here) portion of the Textfield. The spinner is the 🔼 and 🔽 when the type='number'. This is problematic for when we need to implement onClick events around the TextField. This current monorail schema measures whether or not a merchant used the stepper / spinner to adjust their inventory quantity. The event.stopPropagation() no longer allows us to capture this data 😢 and will always return false.

WHAT is this pull request doing?

This PR will conditionally call stopPropagation when click events are captured by TextField and parent elements.

If it is either the spinner or an input, it will no longer call the stopPropagation.

BEFORE

stepper.click.not.propagated.mp4

AFTER

propagation.works.mp4

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';

import {Badge, Page, Stack, TextField} from '../src';

export function Playground() {
  return (
    <Page title="Playground">
      <Stack vertical>
        <div
          onClick={(event) =>
            console.log('Propagated event for the stepper / Spinner', event)
          }
        >
          <TextField
            label="Working Stepper Event"
            autoComplete="off"
            type="number"
          />
        </div>
        <div
          onClick={(event) =>
            console.log('Propagated event / verticalContent', event)
          }
        >
          <TextField
            label="Vertical Content"
            autoComplete="off"
            verticalContent={<Badge>Buyer</Badge>}
          />
        </div>
      </Stack>
    </Page>
  );
}

There should be two TextFields rendered in your playground and one of them contains a Badge as its verticalContent prop.

  • Open your console and click the spinner on the first TextField
  • You should see a log indicating that the click event bubbled up to its parent 🥳
  • On the second TextField click directly on the Badge (not the input field)
  • You should not see any logs indicating that the click event bubbled up (unless you clicked on the input field)

🎩 checklist

@ghost ghost added the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Jul 21, 2022
@ginabak ginabak force-pushed the fix-event-propagation-number branch from 59c1225 to c6857dd Compare July 21, 2022 17:16
@ghost ghost removed the cla-needed Added by a bot. Contributor needs to sign the CLA Agreement. label Jul 21, 2022
@ginabak ginabak force-pushed the fix-event-propagation-number branch from c6857dd to f894d07 Compare July 21, 2022 17:17
@github-actions
Copy link
Contributor

github-actions bot commented Jul 21, 2022

size-limit report 📦

Path Size
polaris-react-cjs 198.29 KB (+0.02% 🔺)
polaris-react-esm 133.18 KB (+0.03% 🔺)
polaris-react-esnext 188.33 KB (+0.02% 🔺)
polaris-react-css 41.75 KB (0%)

return Boolean(multiline) || multiline > 0
? {'aria-multiline': true}
: undefined;
function handleKeyPress(event: React.KeyboardEvent) {
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 just moved these functions to be alphabetical so it's easier to read/find. It shouldn't affect the git blame if I'm not mistaken? 🤞🏽

@ginabak
Copy link
Contributor Author

ginabak commented Jul 25, 2022

/snapit

@ginabak ginabak marked this pull request as ready for review July 25, 2022 14:37
@github-actions
Copy link
Contributor

🫰✨ Thanks @ginabak! Your snapshots have been published to npm.

Test the snapshots by updating your package.json with the newly published versions:

yarn add @shopify/polaris-icons@0.0.0-snapshot-release-20220725143855
yarn add @shopify/polaris@0.0.0-snapshot-release-20220725143855

@ginabak ginabak changed the title [TexField] Allow click event on spinner (stepper in number type) [TextField] Allow click event on spinner (stepper in number type) Jul 25, 2022
'@shopify/polaris': major
---

Allow click events for spinner in [TextField]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spinner (so confusing, I know), is not the actual "loading" spinner, it's this component that has a confusing name, which is the 🔼 and the 🔽 (the stepper) portion of the TextField.

@ginabak ginabak requested a review from shopi-dori July 25, 2022 19:33
Copy link
Contributor

@weslleyaraujo weslleyaraujo left a comment

Choose a reason for hiding this comment

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

awesome! Nice to see this one coming together 🙂

Copy link
Contributor

@LauraAubin LauraAubin left a comment

Choose a reason for hiding this comment

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

🎩 and code looks great!

@@ -629,7 +591,7 @@ export function TextField({
}

function handleClickChild(event: React.MouseEvent) {
if (inputRef.current !== event.target) {
if (!isSpinner(event.target) && !isInput(event.target)) {
Copy link
Member

Choose a reason for hiding this comment

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

Hey @ginabak 👋🏽 Thanks for raising this! I think you will need to also add isSpinner to the conditional on line 581 in the handleClick method to prevent the input focus toggle/flash with each click of the spinner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi hi @chloerice ! GOOD CATCH, OMG! Thank-you! 😄 !

@ginabak ginabak force-pushed the fix-event-propagation-number branch from 8977c12 to 1ef8903 Compare July 27, 2022 14:13
@ginabak ginabak requested a review from chloerice July 27, 2022 14:23
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

🚀

@ginabak ginabak merged commit b75bc19 into main Jul 27, 2022
@ginabak ginabak deleted the fix-event-propagation-number branch July 27, 2022 18:30
@github-actions github-actions bot mentioned this pull request Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TextField]: Events no longer bubble up for Spinner (type='number')
4 participants