Create a SearchSelectorField to replace the current SelectorField for adding skills #663

twilkhoo · 2022-08-28T18:08:33Z

Ticket link

Closes #502

Implementation description

Added a new component, "SearchSelectorField"
The component allows users to type and select existing skills, but also allows them to add new skills
New user-added skills are first stored in a set, from which if they are deleted (volunteer changes their mind), they are removed from the set
Skills are only added to the db when user submits

Steps to test

Login as any user and edit profile
Add skills using the new SearchSelectorField component, which includes selecting existing skills and creating new ones
Save changes, if testing with the firebase hosting link you may get an authentication error since backend changes don't show in the preview
To verify the skills are saved to the db, you can run the contents of this branch locally

What should reviewers focus on?

Ensure that the component works as expected
Please pay attention to minor details too: if a user types a skill that already exists, the option to add that skill disappears. The user input is formatted to titlecase (and trimmed). Clicking in the dropdown keeps the dropdown open, but clicking outside closes it.
Lmk if there's anything that can be made more intuitive

Checklist

My PR name is descriptive and in imperative tense
My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
I have run the appropriate linter(s)
I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

… adding skills

github-actions · 2022-08-28T18:15:04Z

Visit the preview URL for this PR (updated for commit 25ee0d0):

https://sistering-dev--pr663-tejas-update-skills-fgu5cep8.web.app

_{(expires Mon, 05 Sep 2022 22:18:41 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

twilkhoo · 2022-08-28T18:19:17Z

frontend/src/components/user/AccountForm.tsx

-      currentLanguages.filter((l) => l.id !== language),
-    );
-  };
-
  const createAccount = (values: CreateAccountFormValues): void => {
    if (onVolunteerCreate && onEmployeeCreate) {
      if (isAdmin) {


Could someone double-check this part? In editAccount, we set branches: [], but that effectively deletes the branches that were assigned to the user. I've verified this with prisma studio, seems like a bug?

hmm I need to look into this. I've don't think I've ever encountered this bug on staging though

wait yea, I think I get the bug. Definitely worth fixing since it's big impact. I'll file a ticket for it.

I think I can quickly take care of this.

twilkhoo · 2022-08-28T18:22:26Z

frontend/src/components/user/AccountForm.tsx

+    const newSkillsArr: string[] = Array.from(addedSkills);
+    const newSkillsInDB: SkillResponseDTO[] = [];
+
+    for (let i = 0; i < newSkillsArr.length; i += 1) {


forEach doesn't work with await as intended, and for ... of is restricted.

MatoPlus

Woah, good job with the custom component. It definitely seemed non-trivial as expected. I'll try to take a closer look sometime tmw.

MatoPlus · 2022-08-29T21:02:15Z

frontend/src/components/user/AccountForm.tsx

                  tooltip={<Text>Search and select skills you have.</Text>}
                  onSelect={(skill) =>
                    selectSkill(skill, values.skills, setFieldValue)
                  }
                  onDeselect={(skill) =>
                    deselectSkill(skill, values.skills, setFieldValue)
                  }
+                  onCreateNewOption={(newSkill) => addNewSkill(newSkill)}
+                  onDeleteNewOption={(newSkill) => deleteNewSkill(newSkill)}
                />
              )}
              <SelectorField


@twilkhoo it would be really nice to have this for languages as well. Do you think it'll be difficult to include that in this PR as well?

Yeah I was thinking that too! I'm planning to add the language field changes in a separate PR though after this one gets submitted for organization.

frontend/src/components/user/AccountForm.tsx

MatoPlus

Another edge case:

We can't add any skills that are all caps/acronym. I think this is fine though imo since we don't want to give end users too much freedom to add "unknown" acronyms

frontend/src/components/user/AccountForm.tsx

frontend/src/components/user/fields/SearchSelectorField.tsx

MatoPlus

Made some suggestions, interested in your thoughts. Very impressed with the custom component overall 😤

Co-authored-by: Rickson Yang <44826218+MatoPlus@users.noreply.github.com>

twilkhoo · 2022-08-29T22:27:49Z

Regarding the acronyms comment, I suppose that is a limitation with converting everything to title case. If we were to avoid converting to title case however, we would likely have lots of case differences in the db (some users may enter things all lowercase, some all uppercase, etc.). Due to this, I think that keeping the title case conversion outweighs the downsides.

MatoPlus

LGTM, awesome work!

Create a SearchSelectorField to replace the current SelectorField for…

5ea37aa

… adding skills

twilkhoo commented Aug 28, 2022

View reviewed changes

twilkhoo added 2 commits August 28, 2022 15:01

Test changing the authorization

e4604d6

Add volunteer authorization to create skill back

9df573e

twilkhoo marked this pull request as draft August 28, 2022 22:23

twilkhoo marked this pull request as ready for review August 28, 2022 22:44

twilkhoo requested review from neelismail01 and MatoPlus August 28, 2022 22:53

twilkhoo added the awaiting review 👀 label Aug 28, 2022

MatoPlus requested a review from sherryhli August 29, 2022 01:26

MatoPlus reviewed Aug 29, 2022

View reviewed changes

frontend/src/components/user/AccountForm.tsx Show resolved Hide resolved

MatoPlus reviewed Aug 29, 2022

View reviewed changes

frontend/src/components/user/AccountForm.tsx Outdated Show resolved Hide resolved

MatoPlus reviewed Aug 29, 2022

View reviewed changes

frontend/src/components/user/fields/SearchSelectorField.tsx Show resolved Hide resolved

MatoPlus requested changes Aug 29, 2022

View reviewed changes

MatoPlus added changes requested 📝 and removed awaiting review 👀 labels Aug 29, 2022

twilkhoo and others added 2 commits August 29, 2022 18:09

Display user-added values as options in the dropdown

1d11b32

Update frontend/src/components/user/AccountForm.tsx

25ee0d0

Co-authored-by: Rickson Yang <44826218+MatoPlus@users.noreply.github.com>

twilkhoo added awaiting review 👀 and removed changes requested 📝 labels Aug 29, 2022

twilkhoo requested a review from MatoPlus August 29, 2022 22:28

MatoPlus approved these changes Aug 30, 2022

View reviewed changes

MatoPlus added ship it 🚢 and removed awaiting review 👀 labels Aug 30, 2022

twilkhoo merged commit 165c1f6 into main Aug 30, 2022

twilkhoo deleted the tejas/update-skills-field branch August 30, 2022 16:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a SearchSelectorField to replace the current SelectorField for adding skills #663

Create a SearchSelectorField to replace the current SelectorField for adding skills #663

twilkhoo commented Aug 28, 2022 •

edited

Loading

github-actions bot commented Aug 28, 2022 •

edited

Loading

twilkhoo Aug 28, 2022

MatoPlus Aug 29, 2022

MatoPlus Aug 30, 2022

MatoPlus Aug 30, 2022

MatoPlus Aug 30, 2022

twilkhoo Aug 28, 2022

MatoPlus left a comment

MatoPlus Aug 29, 2022

twilkhoo Aug 29, 2022

MatoPlus left a comment

MatoPlus left a comment

twilkhoo commented Aug 29, 2022

MatoPlus left a comment

Create a SearchSelectorField to replace the current SelectorField for adding skills #663

Create a SearchSelectorField to replace the current SelectorField for adding skills #663

Conversation

twilkhoo commented Aug 28, 2022 • edited Loading

Ticket link

Implementation description

Steps to test

What should reviewers focus on?

Checklist

github-actions bot commented Aug 28, 2022 • edited Loading

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

MatoPlus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatoPlus left a comment

Choose a reason for hiding this comment

MatoPlus left a comment

Choose a reason for hiding this comment

twilkhoo commented Aug 29, 2022

MatoPlus left a comment

Choose a reason for hiding this comment

twilkhoo commented Aug 28, 2022 •

edited

Loading

github-actions bot commented Aug 28, 2022 •

edited

Loading