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.

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

Update languages field to use SearchSelectorField #667

Merged
merged 3 commits into from
Aug 30, 2022

Conversation

twilkhoo
Copy link
Contributor

Ticket link

Closes #591, #666

Implementation description

  • Replaced SelectorField with SearchSelectorField for languages in the AccountForm, and added all necessary dependencies, very similar to the skills SearchSelectorField implementation

Steps to test

  1. Login as a volunteer or employee
  2. Navigate to the Edit Profile page and test the languages field
  3. Note: You won't be able to actually submit the changes if using the Firebase hosting link below since that does not include the backend changes in the preview. Testing locally works fine.

What should reviewers focus on?

  • Correctness
  • Similarity with the SearchSelectorField for adding skills
  • Ensuring all code for the languages field implementation doesn't still say "skills" anywhere

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

@@ -181,6 +181,7 @@ const SearchSelectorField = ({
if (option.id !== "-1") { // User-added values have an id of "-1" before submitting
onSelect(option.id);
}
setTextboxInput("");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just clears input when a user selects something from the dropdown.

@github-actions
Copy link

github-actions bot commented Aug 30, 2022

Visit the preview URL for this PR (updated for commit 1237c15):

https://sistering-dev--pr667-tejas-update-languag-exofdmlf.web.app

(expires Tue, 06 Sep 2022 18:47:29 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link
Member

@MatoPlus MatoPlus left a comment

Choose a reason for hiding this comment

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

Code looks good. I'll test this out in a bit and approve.

Copy link
Member

@MatoPlus MatoPlus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding this in. Huge UX improvement 😤

@twilkhoo twilkhoo merged commit d34f3c1 into main Aug 30, 2022
@twilkhoo twilkhoo deleted the tejas/update-languages-field branch August 30, 2022 20:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Languages Dyanmic
2 participants