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

Create a dynamic modal #98

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

michho8
Copy link
Contributor

@michho8 michho8 commented Oct 3, 2024

Ticket Link

Ticket 1802

List of squashed commits

  • Create dynamic modal with an example of how to call it in navbar
  • Allow dynamic number of buttons with example in navbar
  • Started testing, issues with body contents
  • Add tests and remove from navbar
  • Make buttons parameter optional

Test Checklist

  • Unit Tests Passed
  • Integration Tests Passed
  • General Visual Inspection

@michho8 michho8 requested a review from JorWo October 3, 2024 20:33
@JorWo
Copy link
Contributor

JorWo commented Oct 4, 2024

Any plans to create a useModal hook?

@michho8
Copy link
Contributor Author

michho8 commented Oct 15, 2024

Any plans to create a useModal hook?

I use a hook in the file itself, planning to let the parent only call the modal when a certain condition is met. This way the parent components wouldn't have different hooks but rather a boolean (possibly returned from a function) to be toggled in the event of something making a modal pop-up.

If you're talking about actually calling an instance of this dynamic modal, we would need this when we know when we are calling the dynamic modal(s). Each one will have its own conditional clause because they'll depend on different logic. As I looked through the AngularJS UI, I may implement the departmental icon and call the dynamic modal to show the warning message here. I have another ticket that also wants the ApiErrorModal to be replaced with the dynamic modal which I wanted to use as a mentor/mentee ticket.

There's also the option of this boolean hook but I feel strongly for only calling the modal if a conditional statement is met and passing in a boolean value. The dynamic modal should only be opened once when a condition is met and will close using its own hook and not the parent's.

@JorWo
Copy link
Contributor

JorWo commented Oct 15, 2024

Any plans to create a useModal hook?

I use a hook in the file itself, planning to let the parent only call the modal when a certain condition is met. This way the parent components wouldn't have different hooks but rather a boolean (possibly returned from a function) to be toggled in the event of something making a modal pop-up.

If you're talking about actually calling an instance of this dynamic modal, we would need this when we know when we are calling the dynamic modal(s). Each one will have its own conditional clause because they'll depend on different logic. As I looked through the AngularJS UI, I may implement the departmental icon and call the dynamic modal to show the warning message here. I have another ticket that also wants the ApiErrorModal to be replaced with the dynamic modal which I wanted to use as a mentor/mentee ticket.

There's also the option of this boolean hook but I feel strongly for only calling the modal if a conditional statement is met and passing in a boolean value. The dynamic modal should only be opened once when a condition is met and will close using its own hook and not the parent's.

That's a good point, the close button is only needed to be pressed once. I thought it would be useful to have a useModal like this:

const { open, close } = useModal({
    modal: <DynamicModal />,
    open: true, 
    onClose:  () => {}
} );

Anyway a useModal hook is not completely needed, but I noticed that you need to provide your own useState hook to control whether it is open or not, which can be a lot of code repitition. Using a hook, you can extract open and close functions. Definitely something to consider in another ticket.

@michho8 michho8 force-pushed the dev-michelleh-1802 branch from 6a98430 to bada0e3 Compare October 15, 2024 21:55
@michho8 michho8 requested a review from JorWo October 15, 2024 21:57
Copy link
Contributor

@JorWo JorWo left a comment

Choose a reason for hiding this comment

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

Looks good.

@JorWo JorWo merged commit 6b33d1f into uhawaii-system-its-ti-iam:main Oct 15, 2024
6 checks passed
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