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

Add option to disable delete button if model has protected object #573

Merged
merged 10 commits into from
Aug 21, 2023

Conversation

cry-stal-lee
Copy link
Contributor

@cry-stal-lee cry-stal-lee commented Aug 21, 2023

JIRA: https://thatsmighty.atlassian.net/browse/PROD-506
CoOp pull request: https://github.com/mighty-justice/coop-client/pull/1198

Tested in Storybook (ended up removing the Storybook code, thought it might be overkill):
Screenshot 2023-08-21 at 3 18 53 AM

Also tested in CoOp:
Screenshot 2023-08-21 at 3 28 12 AM

@github-actions
Copy link

github-actions bot commented Aug 21, 2023

Pull Request Test Coverage Report for Build 5932108690

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 97.857%

Totals Coverage Status
Change from base Build 5869955311: 0.01%
Covered Lines: 768
Relevant Lines: 771

💛 - Coveralls

Delete
</GuardedButton>
<Tooltip title={hasProtectedObject ? deleteDisabledTooltip : ''}>
<span>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapping it in a <span> is necessary because tooltips don't show on disabled buttons

onDelete?: (model: unknown) => Promise<any>;
protectedField?: string | null;
Copy link
Contributor

@ellenshin ellenshin Aug 21, 2023

Choose a reason for hiding this comment

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

Can this be a boolean field? disableDelete or something and on the frontend you can just pass {!!payments}. I think that makes the prop a little more straight forward but also we would be able to use that prop for when we just want to disable the delete button for other reasons (i.e. disable if the user doesn't have the permission to delete.

Copy link
Contributor Author

@cry-stal-lee cry-stal-lee Aug 21, 2023

Choose a reason for hiding this comment

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

Each investment could have or not have payments, so we have to pass the field along with the model to EditableArrayCard so when it renders each EditableCard it can check whether that instance has payments or not. From inside coop-client we can't pass a boolean for each investment since the frontend only passes the model. Let me know if I'm understanding you correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the future we could do a disableDelete for permissions since we could just disable the delete button for the entire EditableArrayCard

Copy link
Contributor

@ellenshin ellenshin Aug 21, 2023

Choose a reason for hiding this comment

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

this prop feels way too specific for the component i think. Do you think this is possible?

<EditableArrayCard
...
renderDelete={(investment) => investment.payments && investment.payments.length > 0}
...
/>
const enableDelete = renderDelete ? renderDelete(model) : true;

something like that... then we can just pass (investment) => false to the prop if we want to disable it for other reasons?

Copy link
Contributor

Choose a reason for hiding this comment

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

also didnt realize that this was a array card! but i think that might work?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah agreed! I think passing the function is much better. updated!

>
Delete
</GuardedButton>
<Tooltip title={isDeleteDisabled ? disabledDeleteTooltip : ''}>
Copy link
Contributor

Choose a reason for hiding this comment

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

if the button is disabled but the prop passed an empty string for disabledDeleteTooltip, does the tool tip render with an empty string or does not render at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No tooltip is rendered when the string is empty!

@cry-stal-lee cry-stal-lee merged commit f445600 into master Aug 21, 2023
1 check passed
@cry-stal-lee cry-stal-lee deleted the prod-506/render-coop-alert branch August 21, 2023 22:45
cry-stal-lee added a commit that referenced this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants