Skip to content

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

ModalComponent?: new (props: ISharedFormModalProps) => FormModal | FormDrawer;
deleteDisabledTooltip?: string;
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
@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.

3 participants