🪟 🐛 Fix tag input styles #20020

flash1293 · 2022-12-02T14:36:47Z

What

Before:

After (regular, hovered, focused, error):

How

Upgrading react-select made it necessary to overwrite a bunch of additional styles because react-select became more opinionated. This was necessary before, so the existing custom styles could easily be extended.

I couldn't figure out a native way to trigger the error state (I most likely missed it, but even if not we might want to add it at a later point in time), making it required by adding

          .required("you need this!")

to

airbyte/airbyte-webapp/src/core/jsonSchema/schemaToYup.ts

Line 85 in e3ef3fd

.required("you need this!")

makes it easy to do so in a synthetic way.

Drive-by changes

Removed some any-s, seems like the react-select typings got better.
Set aria-invalid on the react-select if it's in error state.

🚨 User Impact 🚨

Fixes the visual bug, no changes in behavior.

lmossman · 2022-12-02T19:20:08Z

I couldn't figure out a native way to trigger the error state

For the Postgres source, it is not showing the Required error state when empty because it is an optional field, so the yup .required() is not being applied here:

airbyte/airbyte-webapp/src/core/jsonSchema/schemaToYup.ts

Lines 145 to 147 in f3d4830

    
           if (schema && isRequired) { 
        
             schema = schema.required("form.empty.error"); 
        
           }

We may have other connectors with string array fields that are required, which should be using this error state, but I don't know off-hand which ones

lmossman

Had a couple comments coming from testing locally. Otherwise the code changes look good

lmossman · 2022-12-02T19:25:10Z

airbyte-webapp/src/components/ui/TagInput/TagInput.tsx

    ...provided,
    borderRadius: `${styles.borderRadius}`,
  }),
+  control: (provided, state) => {


One thing I noticed when testing locally was that when I hovered my cursor over the TagInput it stays as the default cursor, whereas when I hover over the other text input fields it changes to a text cursor. Could we also make this TagInput have a text cursor, so that it is consistent? (And I feel a text cursor also makes sense here because the user is still expected to type values into the field)

good catch, fixed

@flash1293 one more small nit here: when hovering over the X close button of one of the tags, could we go back to the default cursor (or maybe pointer, since that seems to be what we use for buttons)? It currently uses the text cursor which looks a little odd.

We should also use the same pointer style for the whole-component clear X button on the right side

lmossman · 2022-12-02T19:28:20Z

airbyte-webapp/src/components/ui/TagInput/TagInput.tsx

    ...provided,
    borderRadius: `${styles.borderRadius}`,
  }),
+  control: (provided, state) => {


Another thing I noticed while testing locally: this TagInput requires users to type in a value and then hit Enter for that value to be actually added to the field as a tag. If the users just types a value and then clicks away, the value they typed is erased.

I feel like this is not a very clear UX that the user has to press Enter in order for their changes to actually be saved, and it would be better if unfocusing the input also added their text as a tag. I think that behavior actually might have existed before we upgraded the react-select version.

Could you explore adding that functionality here? Also fine if you'd rather address that in a separate PR.

I agree, it can be very annoying to a user, especially as it works fine when navigating via keyboard. I didn't find a "supported" way, but I think I came up with an acceptable workaround.

Tried it out and I think this looks good! One question:

This needs to be implemented outside of the onBlur prop of react-select because it's not default behavior.

What do you mean by "it's not default behavior"? Isn't the handleDelete method we are passing to the onBlur prop also not default behavior?

flash1293 · 2022-12-05T10:29:08Z

@lmossman could you check again?

lmossman

Had one more small comment about cursor styling but otherwise LGTM!

Tested out locally and looks to be working well

lmossman · 2022-12-05T18:13:52Z

airbyte-webapp/src/components/ui/TagInput/TagInput.tsx

    ...provided,
    borderRadius: `${styles.borderRadius}`,
  }),
+  control: (provided, state) => {


@flash1293 one more small nit here: when hovering over the X close button of one of the tags, could we go back to the default cursor (or maybe pointer, since that seems to be what we use for buttons)? It currently uses the text cursor which looks a little odd.

We should also use the same pointer style for the whole-component clear X button on the right side

lmossman · 2022-12-05T18:21:56Z

airbyte-webapp/src/components/ui/TagInput/TagInput.tsx

    ...provided,
    borderRadius: `${styles.borderRadius}`,
  }),
+  control: (provided, state) => {


Tried it out and I think this looks good! One question:

This needs to be implemented outside of the onBlur prop of react-select because it's not default behavior.

What do you mean by "it's not default behavior"? Isn't the handleDelete method we are passing to the onBlur prop also not default behavior?

flash1293 · 2022-12-05T19:04:28Z

What do you mean by "it's not default behavior"? Isn't the handleDelete method we are passing to the onBlur prop also not default behavior?

I just mean that the onBlur prop of the component is not called when you are actually bluring, that's why I had to add it to the surrounding div and wait for the event to bubble up. Just a technical detail.

Joe Reuter added 2 commits December 2, 2022 15:29

fix input styles

e3ef3fd

clean up

f375684

octavia-squidington-iv added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Dec 2, 2022

flash1293 changed the title ~~Fix tag input styles~~ 🪟 🐛 Fix tag input styles Dec 2, 2022

Merge remote-tracking branch 'upstream/master' into tag-input-style

0e96257

flash1293 marked this pull request as ready for review December 2, 2022 16:51

flash1293 requested a review from a team as a code owner December 2, 2022 16:51

lmossman reviewed Dec 2, 2022

View reviewed changes

Joe Reuter added 2 commits December 5, 2022 11:10

Merge remote-tracking branch 'upstream/master' into tag-input-style

f301026

review comments

dc5e73b

lmossman approved these changes Dec 5, 2022

View reviewed changes

Joe Reuter added 2 commits December 6, 2022 11:28

Merge remote-tracking branch 'upstream/master' into tag-input-style

a2e0aaa

pointer cursor for the remove buttons

e5aff74

flash1293 merged commit 9007271 into master Dec 6, 2022

flash1293 deleted the tag-input-style branch December 6, 2022 13:36

This was referenced Dec 6, 2022

Bump Airbyte version from 0.40.23 to 0.40.24 #20161

Closed

Bump Airbyte version from 0.40.23 to 0.40.24 #20206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟 🐛 Fix tag input styles #20020

🪟 🐛 Fix tag input styles #20020

flash1293 commented Dec 2, 2022

lmossman commented Dec 2, 2022

lmossman left a comment

lmossman Dec 2, 2022

flash1293 Dec 5, 2022

lmossman Dec 5, 2022

lmossman Dec 2, 2022

flash1293 Dec 5, 2022

lmossman Dec 5, 2022

flash1293 commented Dec 5, 2022

lmossman left a comment

lmossman Dec 5, 2022

lmossman Dec 5, 2022

flash1293 commented Dec 5, 2022

🪟 🐛 Fix tag input styles #20020

🪟 🐛 Fix tag input styles #20020

Conversation

flash1293 commented Dec 2, 2022

What

How

Drive-by changes

🚨 User Impact 🚨

lmossman commented Dec 2, 2022

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Dec 5, 2022

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Dec 5, 2022