-
Notifications
You must be signed in to change notification settings - Fork 111
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
Reset modal after adding event #442
base: main
Are you sure you want to change the base?
Conversation
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 job. This should do the trick while we wait to start on a redesigned event modal. 👍
<Backdrop | ||
onClick={() => { | ||
context.handleClose(); | ||
if (typeof onBackdropClick === "function") onBackdropClick(); |
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.
I don't think this conditional should silently pass if a "function" typed onBackdropClick
isn't passed as a prop.
If someone is adding more modals later but accidentally passes an onBackdropClick
of some type other than "function", this if
will essentially hide that error.
Instead, it might be better to just check for the existence of onBackdropClick
. Then if a non-function prop is passed to Modal
it will easily be caught during testing.
if (typeof onBackdropClick === "function") onBackdropClick(); | |
if (onBackdropClick) onBackdropClick(); |
@@ -2,7 +2,7 @@ import { AnimatePresence } from "framer-motion"; | |||
import Backdrop from "./Backdrop"; | |||
import { motion } from "framer-motion"; | |||
|
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.
Optionally you could add a check for the onBackdropClick
type with prop-types
. That way you get an error on page load if an invalid onBackdropClick
prop is passed in instead of at runtime.
import PropTypes from "prop-types"; | |
// Modal component definition | |
Modal.propTypes = { | |
onBackdropClick: PropTypes.func, | |
}; |
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 a redesign for the event modal is in the works like @intelagense mentioned then this may not be that important. Just my two cents.
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 a redesign for the event modal is in the works like @intelagense mentioned then this may not be that important. Just my two cents.
I'm not sure the redesign will be coming anytime soon.
Description
Reset the form after adding an event by clicking the 'close' button or clicking on the modal backdrop.
Type of change
Please select everything applicable. Please, do not delete any lines.
Issue
Checklist:
npm run test
and all tests have passed successfully or I have included details within my PR on the failure.npm run lint
and resolved any outstanding errors. Most issues can be solved by executingnpm run format