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

🐛 fix(autocomplete): tags not generated for preselected options #1403

Merged
merged 7 commits into from
Sep 27, 2023
Merged

🐛 fix(autocomplete): tags not generated for preselected options #1403

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
740f388
fix(autocomplete): tags not generated for preselected options
gitdallas Sep 26, 2023
16b6d20
cleanup commented code
gitdallas Sep 26, 2023
431d8b4
cleanup, stick with selection naming rather than sometimes chip
gitdallas Sep 26, 2023
53069c0
filter out tags that have a non-falsey source value
gitdallas Sep 26, 2023
ba418c1
use set for tag options, filter shown tags with other source
gitdallas Sep 27, 2023
d2cb75b
remove console log
gitdallas Sep 27, 2023
7a832ca
Merge branch 'main' into fix/j1315-existing-autocomplete-tags-not-sho…
ibolton336 Sep 27, 2023
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
50 changes: 22 additions & 28 deletions client/src/app/components/Autocomplete.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,13 @@ export const Autocomplete: React.FC<IAutocompleteProps> = ({
}) => {
const [inputValue, setInputValue] = useState(searchString);
const [menuIsOpen, setMenuIsOpen] = useState(false);
const [currentChips, setCurrentChips] = useState<Set<string>>(
new Set(selections)
);
const [hint, setHint] = useState("");
const [menuItems, setMenuItems] = useState<React.ReactElement[]>([]);

/** refs used to detect when clicks occur inside vs outside of the textInputGroup and menu popper */
const menuRef = useRef<HTMLDivElement>(null);
const searchInputRef = useRef<HTMLInputElement>(null);

React.useEffect(() => {
onChange([...currentChips]);
buildMenu();
}, [currentChips]);

React.useEffect(() => {
buildMenu();
}, [options]);
Expand All @@ -67,7 +59,7 @@ export const Autocomplete: React.FC<IAutocompleteProps> = ({
.filter(
(item: string, index: number, arr: string[]) =>
arr.indexOf(item) === index &&
!currentChips.has(item) &&
!selections.includes(item) &&
(!inputValue || item.toLowerCase().includes(inputValue.toLowerCase()))
)
.map((currentValue, index) => (
Expand Down Expand Up @@ -127,33 +119,32 @@ export const Autocomplete: React.FC<IAutocompleteProps> = ({
buildMenu();
};

/** callback for removing a chip from the chip selections */
const deleteChip = (chipToDelete: string) => {
const newChips = new Set(currentChips);
newChips.delete(chipToDelete);
setCurrentChips(newChips);
/** callback for removing a selection */
const deleteSelection = (selectionToDelete: string) => {
onChange(selections.filter((s) => s !== selectionToDelete));
};

/** add the given string as a chip in the chip group and clear the input */
const addChip = (newChipText: string) => {
/** add the given string as a selection */
const addSelection = (newSelectionText: string) => {
if (!allowUserOptions) {
const matchingOption = options.find(
(o) => o.toLowerCase() === (hint || newChipText).toLowerCase()
(o) => o.toLowerCase() === (hint || newSelectionText).toLowerCase()
);
if (!matchingOption) {
console.log({ matchingOption, newSelectionText, options });
if (!matchingOption || selections.includes(matchingOption)) {
return;
}
newChipText = matchingOption;
newSelectionText = matchingOption;
}
setCurrentChips(new Set([...currentChips, newChipText]));
onChange([...selections, newSelectionText]);
setInputValue("");
setMenuIsOpen(false);
};

/** add the current input value as a chip */
/** add the current input value as a selection */
const handleEnter = () => {
if (inputValue.length) {
addChip(inputValue);
addSelection(inputValue);
}
};

Expand Down Expand Up @@ -216,13 +207,13 @@ export const Autocomplete: React.FC<IAutocompleteProps> = ({
closeMenu && setMenuIsOpen(false);
};

/** add the text of the selected item as a new chip */
/** add the text of the selected menu item to the selected items */
const onSelect = (event?: React.MouseEvent<Element, MouseEvent>) => {
if (!event) {
return;
}
const selectedText = (event.target as HTMLElement).innerText;
addChip(selectedText);
addSelection(selectedText);
event.stopPropagation();
focusTextInput(true);
};
Expand Down Expand Up @@ -301,10 +292,13 @@ export const Autocomplete: React.FC<IAutocompleteProps> = ({
</FlexItem>
<FlexItem key="chips">
<Flex spaceItems={{ default: "spaceItemsXs" }}>
{Array.from(currentChips).map((currentChip) => (
<FlexItem key={currentChip}>
<Label color={labelColor} onClose={() => deleteChip(currentChip)}>
{currentChip}
{selections.map((currentSelection) => (
<FlexItem key={currentSelection}>
<Label
color={labelColor}
onClose={() => deleteSelection(currentSelection)}
>
{currentSelection}
</Label>
</FlexItem>
))}
Expand Down
116 changes: 38 additions & 78 deletions client/src/app/pages/applications/components/application-form/application-form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,14 @@ export const ApplicationForm: React.FC<ApplicationFormProps> = ({
}, []);

const tagOptions =
tags?.map((tag) => {
return {
value: tag.name,
toString: () => tag.name,
};
}) || [];
tags
?.filter((tag) => !tag.source)
.map((tag) => {
return {
value: tag.name,
toString: () => tag.name,
};
}) || [];

const getBinaryInitialValue = (
application: Application | null,
Expand Down Expand Up @@ -445,78 +447,36 @@ export const ApplicationForm: React.FC<ApplicationFormProps> = ({
name="tags"
label={t("terms.tags")}
fieldId="tags"
renderInput={({ field: { value, name, onChange } }) => (
<Autocomplete
noResultsMessage={t("message.noResultsFoundTitle")}
onChange={(selections) => {
onChange(
selections
.map((sel) => getTagRef(sel))
.filter((sel) => sel !== undefined) as TagRef[]
);
}}
options={tagOptions.map((o) => o.value)}
placeholderText={t("composed.selectMany", {
what: t("terms.tags").toLowerCase(),
})}
searchInputAriaLabel="tags-select-toggle"
selections={
value
.map(
(formTag) =>
tags?.find((tagRef) => tagRef.name === formTag.name)
)
.map((matchingTag) =>
matchingTag ? matchingTag.name : undefined
)
.filter((e) => e !== undefined) as string[]
}
/>
// <SimpleSelect
// maxHeight={DEFAULT_SELECT_MAX_HEIGHT}
// placeholderText={t("composed.selectMany", {
// what: t("terms.tags").toLowerCase(),
// })}
// id="tags-select"
// variant="typeaheadmulti"
// toggleId="tags-select-toggle"
// toggleAriaLabel="tags dropdown toggle"
// aria-label={name}
// value={value
// .map((formTag) =>
// tags?.find((tagRef) => tagRef.name === formTag.name)
// )
// .map((matchingTag) =>
// matchingTag
// ? {
// value: matchingTag.name,
// toString: () => matchingTag.name,
// }
// : undefined
// )
// .filter((e) => e !== undefined)}
// options={tagOptions}
// onChange={(selection) => {
// const selectionWithValue = selection.toString();

// const currentValue = value || [];
// const e = currentValue.find(
// (f) => f.name === selectionWithValue
// );
// if (e) {
// onChange(
// currentValue.filter((f) => f.name !== selectionWithValue)
// );
// } else {
// const tag = getTagRef(selectionWithValue);
// if (currentValue && typeof tag !== "undefined")
// onChange([...currentValue, tag]);
// }
// }}
// onClear={() => onChange([])}
// noResultsFoundText={t("message.noResultsFoundTitle")}
// />
)}
renderInput={({ field: { value, name, onChange } }) => {
const selections = value
.map(
(formTag) =>
tags?.find((tagRef) => tagRef.name === formTag.name)
)
.map((matchingTag) =>
matchingTag ? matchingTag.name : undefined
)
.filter((e) => e !== undefined) as string[];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const selections = value
.map(
(formTag) =>
tags?.find((tagRef) => tagRef.name === formTag.name)
)
.map((matchingTag) =>
matchingTag ? matchingTag.name : undefined
)
.filter((e) => e !== undefined) as string[];
const tagNames = new Set(tags?.map((tag) => tag.name));
const selections = value.flatMap((formTag) =>
formTag.source === "" && tagNames.has(formTag.name)
? [formTag.name]
: []
);

Copy link
Member

@ibolton336 ibolton336 Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Played around with this a bit and borrowed Mikes suggestion of using a Set/has here instead of creating a map and using includes since set narrows the tags to unique values & boosts performance. Explicitly checking for the empty string source on the form tag on lookup should give us what we want since empty string source here means "manually selected tag" for a tag on an application object (a formTag in this case) .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ended up making tagOptions a set of strings (tag names) from the beginning and using that. Also as the onChange event maps the form values to tags (which seems to have undefined source instead of ""), I merge a {source:""} object when returning from getTagRef. Without this, after making any changes to the value we'd filter them out if they didn't have a source prop in the tags array.

Copy link
Member

Choose a reason for hiding this comment

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

Noticing that onSubmit, any tags that are filtered out from the selections on line 448 are not being added back in to the payload.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, looks like even when we patch the object with the "archetype tags" not included, the hub just adds them back anyway. Need to check with Sam on this.

Copy link
Member

Choose a reason for hiding this comment

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

Sam confirmed that there is no way for us to overwrite those non-manual tags by updating the application object directly so we should be good on what we are passing to the put request here. There seems to be some ongoing discussion/weirdness around this since the non-manual tags are "virtual" tags being inherited from other membership criteria etc.

We do need to make sure we are filtering out any pre-existing, non-manual tags in the selectable tag list so that we can't accidentally create a new "manual" tag for a tag that already exists on the application from a different source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm unsure if there is a change you're asking for or if you are saying to wait.

Right now it is using tags (filtered to only include undefined or "" source value) as the available options and not letting the user add new tags that don't exist outside the options.

Copy link
Member

Choose a reason for hiding this comment

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

Understood - My concern was that after we are filtering out the tags on an app with a source other than "", we never add them back in to the tags payload on submit so we'd be overwriting those tags.

Copy link
Member

@ibolton336 ibolton336 Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right now it is using tags (filtered to only include undefined or "" source value) as the available options and not letting the user add new tags that don't exist outside the options.

The change I think we might temporarily need is to check to see if any tags in the "tagOptions" list you've created match up with any other tags on the application (archetype source or other non-manual tags) and remove any matching available tag from the list. The tags api fetch response won't include any tags with other sources . Those sources are only on tagRefs for tags that have been associated with an application or archetype on their respective api object. We would have to cross check the response from the tags fetch api with what exists on the application in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Example:

const filteredTagOptions = new Set(
  [...tagOptions].filter(
    (tagOption) => {
      const matchingAppTag = application.tags.find((appTag) => appTag.name === tagOption);
      if (!matchingAppTag) return true;
      if (matchingAppTag.source === '') return true;
      return false;
    }
  )
);


return (
<Autocomplete
noResultsMessage={t("message.noResultsFoundTitle")}
onChange={(selections) => {
onChange(
selections
.map((sel) => getTagRef(sel))
.filter((sel) => sel !== undefined) as TagRef[]
);
}}
options={tagOptions.map((o) => o.value)}
placeholderText={t("composed.selectMany", {
what: t("terms.tags").toLowerCase(),
})}
searchInputAriaLabel="tags-select-toggle"
selections={selections}
/>
);
}}
/>
<HookFormPFGroupController
control={control}
Expand Down