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

refactor: remove instances of set and refactor to accept arrays #1047

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

refactor: remove instances of set and refactor to accept arrays #1047

wants to merge 4 commits into from

Conversation

prestonb91
Copy link

@prestonb91 prestonb91 commented Feb 15, 2025 •
edited
Loading

✅ Resolves #999

  • PR title follows Conventional Commits specifications
  • PR assignee has been selected
  • PR label has been selected
  • @NabbeunNabi has been selected for preliminary review

🔧 What changed

  • refactored ModSearchBar.vue, ModEditHealthcareProfessionalSection.vue, healthcareProfessionalsStore.ts to accept and declare array type

🧪 Testing instructions

📸 Screenshots

Healthcare-professional-duplicate-degrees-1
Healthcare-professional-duplicate-degrees-2
Healthcare-professional-filtered-list-1
Locale-search-bug-1
Locale-search-bug-2

Before

  • ModSearchBar.vue, ModEditHealthcareProfessionalSection.vue, healthcareProfessionalsStore.ts would declare and set the type of facility, locale, insurance, degree, speciality of healthcare professionals in the moderation panel as sets
  • Data that is being passed are already unique meaning arrays instead of sets are sufficient and simplifies code instead of having to convert back and forth with arrays and sets

After

  • Refactored types of and any declared new Set() as arrays between the 3 files

Preliminary Review Request

  1. After refactoring to arrays, healthcare professional Yoshida/吉田 comes up with duplicate degrees. (screenshot # 1, 2) Doesn't occur with other professionals where search query returns filtered items. (screenshot # 3)
  2. Searching locales only works up to locale abbreviation. May be a preexisting bug requiring a separate ticket. (screenshot # 4, 5)

@netlify Netlify
Copy link

netlify bot commented Feb 15, 2025 •
edited
Loading

✅ Deploy Preview for findadoc ready!

Name Link
🔨 Latest commit a9b6b36
🔍 Latest deploy log https://app.netlify.com/sites/findadoc/deploys/67b43dfbe119500008b613e7
😎 Deploy Preview https://deploy-preview-1047--findadoc.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@prestonb91 prestonb91 self-assigned this Feb 16, 2025
@prestonb91
Copy link
Author

@NabbeunNabi , could I ask for a preliminary review for this PR?

I believe have set instances and types from sets to arrays but came across 2 issues wanted to confirm.

  1. After refactoring to arrays, healthcare professional Yoshida/吉田 comes up with duplicate degrees. (screenshot # 1, 2) Doesn't occur with other professionals where search query returns filtered items. (screenshot # 3) Wonder if this is an issue with this issue's code or if it is a separate issue where fake data is assigning duplicates when supposed to be unique? If may be issue with this ticket, I can look into further.
  2. Searching locales only works up to locale abbreviation. This was the case before refactoring for this ticket so was wondering if should set a separate ticket so searching works across full language name search. (screenshot # 5, 6)

@NabbeunNabi
Copy link
Contributor

@NabbeunNabi , could I ask for a preliminary review for this PR?

I believe have set instances and types from sets to arrays but came across 2 issues wanted to confirm.

  1. After refactoring to arrays, healthcare professional Yoshida/吉田 comes up with duplicate degrees. (screenshot # 1, 2) Doesn't occur with other professionals where search query returns filtered items. (screenshot # 3) Wonder if this is an issue with this issue's code or if it is a separate issue where fake data is assigning duplicates when supposed to be unique? If may be issue with this ticket, I can look into further.
  2. Searching locales only works up to locale abbreviation. This was the case before refactoring for this ticket so was wondering if should set a separate ticket so searching works across full language name search. (screenshot # 5, 6)

@prestonb91 number 2 is very valid. It is definitely searching by the locale code and displaying the language. so it will only work for the first two letters. This should be updated where you can type the language but the value is the language code. If that makes sense why don't you write a separate issue for that. I will look into number 1. It definitely could be that it was fake data that duplicated the same degree twice to start

@NabbeunNabi
Copy link
Contributor

It isn't letting me click on the line but 41 has an error. You can fix it by changing it in the ModEditHealthcareProfessionalSection.vue to

v-model="nameLocaleInputs.middleName as string"

NabbeunNabi
NabbeunNabi requested changes Feb 17, 2025
View reviewed changes
Copy link
Contributor

@NabbeunNabi NabbeunNabi left a comment

Choose a reason for hiding this comment

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

@prestonb91 Great start. Just one change at the moment for how the values are spliced. They are added properly. I think the double degree was probably the autogenerated data if you hadn't clicked and updated anything

@NabbeunNabi
Copy link
Contributor

Also do not forget that when a code review is ready to also post it in slack and ping @approvers-frontend

@NabbeunNabi
Copy link
Contributor

image
Also the tags should match the ticket and don't forget to choose the project and that it is in review like the following one.

image

@prestonb91 prestonb91 added enhancement New feature or request good first issue Good for newcomers pair programming This is a ticket that is good to pair with someone on labels Feb 17, 2025
@prestonb91 prestonb91 force-pushed the refactor/modsearch.bar-to-accept-arrays branch from b7d5a00 to 97bb6d8 Compare February 18, 2025 05:47
@prestonb91 prestonb91 marked this pull request as ready for review February 18, 2025 05:57
NabbeunNabi
NabbeunNabi requested changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@NabbeunNabi NabbeunNabi left a comment

Choose a reason for hiding this comment

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

@prestonb91 ALMOST THERE! 👯‍♂️

@prestonb91 prestonb91 force-pushed the refactor/modsearch.bar-to-accept-arrays branch from 97bb6d8 to a9b6b36 Compare February 18, 2025 07:59
NabbeunNabi
NabbeunNabi approved these changes Feb 18, 2025
View reviewed changes
Copy link
Contributor

@NabbeunNabi NabbeunNabi left a comment

Choose a reason for hiding this comment

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

Great work.

Onto @ermish

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NabbeunNabi NabbeunNabi NabbeunNabi approved these changes

@theyokohamalife theyokohamalife Awaiting requested review from theyokohamalife theyokohamalife is a code owner

@ermish ermish Awaiting requested review from ermish ermish is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@prestonb91 prestonb91

Labels
enhancement New feature or request good first issue Good for newcomers moderation panel pair programming This is a ticket that is good to pair with someone on
Projects
None yet
Milestone
No milestone
2 participants
@prestonb91 @NabbeunNabi