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.

Sign up for GitHub

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

Jump to bottom

🪟 🐛 Fix tag input styles #20020

Merged
merged 7 commits into from
Dec 6, 2022
Merged

🪟 🐛 Fix tag input styles #20020

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e3ef3fd
fix input styles
Dec 2, 2022
f375684
clean up
Dec 2, 2022
0e96257
Merge remote-tracking branch 'upstream/master' into tag-input-style
Dec 2, 2022
f301026
Merge remote-tracking branch 'upstream/master' into tag-input-style
Dec 5, 2022
dc5e73b
review comments
Dec 5, 2022
a2e0aaa
Merge remote-tracking branch 'upstream/master' into tag-input-style
Dec 6, 2022
e5aff74
pointer cursor for the remove buttons
Dec 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions airbyte-webapp/src/components/ui/TagInput/TagInput.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@

:export {
backgroundColor: colors.$dark-blue-800;
inputBackgroundColor: colors.$grey-50;
focusedBorderColor: colors.$blue-400;
hoveredBorderColor: colors.$grey-100;
errorBorderColor: colors.$red-100;
errorHoveredBorderColor: colors.$red-300;
fontColor: colors.$white;
borderRadius: variables.$border-radius-md;
paddingLeft: variables.$spacing-sm;
Expand Down
31 changes: 22 additions & 9 deletions airbyte-webapp/src/components/ui/TagInput/TagInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import uniqueId from "lodash/uniqueId";
import { KeyboardEventHandler, useMemo, useState } from "react";
import { ActionMeta, MultiValue, OnChangeValue } from "react-select";
import { ActionMeta, GroupBase, MultiValue, OnChangeValue, StylesConfig } from "react-select";
import CreatableSelect from "react-select/creatable";

import styles from "./TagInput.module.scss";
Expand All @@ -9,9 +9,8 @@ const components = {
DropdownIndicator: null,
};

const customStyles = {
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- react-select's typing is lacking here
multiValue: (provided: any) => ({
const customStyles: StylesConfig<Tag, true, GroupBase<Tag>> = {
multiValue: (provided) => ({
...provided,
maxWidth: "100%",
display: "flex",
Expand All @@ -20,17 +19,29 @@ const customStyles = {
borderRadius: `${styles.borderRadius}`,
paddingLeft: `${styles.paddingLeft}`,
}),
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- same as above
multiValueLabel: (provided: any) => ({
multiValueLabel: (provided) => ({
...provided,
color: `${styles.fontColor}`,
fontWeight: 500,
}),
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- same as above
multiValueRemove: (provided: any) => ({
multiValueRemove: (provided) => ({
...provided,
borderRadius: `${styles.borderRadius}`,
}),
control: (provided, state) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

const isInvalid = state.selectProps["aria-invalid"];
const regularBorderColor = isInvalid ? styles.errorBorderColor : "transparent";
const hoveredBorderColor = isInvalid ? styles.errorHoveredBorderColor : styles.hoveredBorderColor;
return {
...provided,
backgroundColor: styles.inputBackgroundColor,
boxShadow: "none",
borderColor: state.isFocused ? styles.focusedBorderColor : regularBorderColor,
":hover": {
borderColor: state.isFocused ? styles.focusedBorderColor : hoveredBorderColor,
},
};
},
};

interface Tag {
Expand All @@ -56,7 +67,7 @@ const generateStringFromTag = (tag: Tag): string => tag.label;

const delimiters = [",", ";"];

export const TagInput: React.FC<TagInputProps> = ({ onChange, fieldValue, name, disabled, id }) => {
export const TagInput: React.FC<TagInputProps> = ({ onChange, fieldValue, name, disabled, id, error }) => {
const tags = useMemo(() => fieldValue.map(generateTagFromString), [fieldValue]);

// input value is a tag draft
Expand Down Expand Up @@ -120,6 +131,8 @@ export const TagInput: React.FC<TagInputProps> = ({ onChange, fieldValue, name,
name={name}
components={components}
inputValue={inputValue}
placeholder=""
aria-invalid={Boolean(error)}
isClearable
isMulti
onBlur={() => handleDelete}
Expand Down
2 changes: 1 addition & 1 deletion airbyte-webapp/src/views/Connector/ConnectorForm/components/Property/Control.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const Control: React.FC<ControlProps> = ({
name={name}
fieldValue={field.value || []}
onChange={(tagLabels) => helpers.setValue(tagLabels)}
// error={!!meta.error}
error={!!meta.error}
disabled={disabled}
/>
)}
Expand Down