-
Notifications
You must be signed in to change notification settings - Fork 0
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
EHPR-230 Added delete functionality to each specialty row #165
Conversation
@@ -124,6 +124,8 @@ export const Credential: React.FC = () => { | |||
index={index} | |||
specialties={specialtyOptions} | |||
subspecialties={subspecialties?.[index]} | |||
deleteFunction={() => arrayHelpers.remove(index)} | |||
enableDelete={specialties.length > 1} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The delete function handles the removal of the specialty.
The enable delete determines whether of not the delete button is shown.
I put them at a higher level and pass them down into the component to keep the SpecialtySelector
component generic and because otherwise I would need to pass the specialties
array and arrayHelper
function into the component.
<button | ||
type='button' | ||
className='text-bcRedError' | ||
aria-label='delete the last specialty' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now should it be n-th specialty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! i will update this
@@ -221,6 +212,16 @@ const SpecialtySelector: React.FC<SpecialtySelectorProps> = ({ | |||
/> | |||
))} | |||
</Select> | |||
{enableDelete ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we want disallow deleting the last specialty, can we just check if index is 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to make sure that you can delete any specialty selection, unless there is only one left.
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
See post on slack for video on functionality