segmentio / evergreen

🌲 Evergreen React UI Framework by Segment
https://evergreen.segment.com
MIT License
12.39k stars 832 forks source link

fix esc on tagInput component #1473

Closed pandurijal closed 2 years ago

pandurijal commented 2 years ago

Overview Resolves #1472

The proposed behavior for pressing esc:

Screenshots (if applicable)

Documentation

netlify[bot] commented 2 years ago

Deploy Preview for evergreen-storybook ready!

Name Link
Latest commit 03ea13a0089a35fdf0c348255fef59fc17213bc0
Latest deploy log https://app.netlify.com/sites/evergreen-storybook/deploys/628d65d51bbcee00084a7d49
Deploy Preview https://deploy-preview-1473--evergreen-storybook.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

brandongregoryscott commented 2 years ago

Hey @pandurijal! Thanks for the contribution. I was in the middle of typing up a response about some issues I encountered and it looks like your most recent commit resolved them! (and fixed the failing tests - glad it was caught!) Gonna do some additional smoke testing in storybook before I merge it in, but this is looking good now.

Edit: I am still seeing some issues in the base Autocomplete component. Pressing esc throws a different error now: ezgif com-gif-maker (2)

For what it's worth, I was thinking a simpler check against a null or undefined value in the getValues function would suffice:

https://github.com/segmentio/evergreen/blob/7b2122548a6217c1263dbe9f364b32dc965b3c91/src/tag-input/src/TagInput.js#L69-L75

The inputValue = '' doesn't seem to catch null being passed in from event.target.value, but I imagine this would guard against it:

const getValues = (inputValue = "") => {
  inputValue = inputValue ?? "";

  return separator
    ? inputValue
        .split(separator)
        .map((v) => v.trim())
        .filter((v) => v.length > 0)
    : [inputValue];
};
pandurijal commented 2 years ago

Hi @brandongregoryscott!

Well yeah, I thought the issue was from the Downshift component, turns out your approach could fix the issue without having any additional effect on another component(s). So I think I would go with your solution after testing some more things I will push the changes.