-
Notifications
You must be signed in to change notification settings - Fork 7
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
#168105593 Reusable modal component #274
base: develop
Are you sure you want to change the base?
Conversation
53365aa
to
2348203
Compare
clientV2/components/modals/Modal.jsx
Outdated
toggle = 'flex'; | ||
} | ||
return ( | ||
<div className="modal" style={{ display: toggle }}> |
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 think it's not a good practice to use style directly this way, because of it's over-riding behaviour. I think a better approach is to create class with display property of none and another with display property of flex in your _modal.scss. Then you can do something like
<div className={
modal ${openModal ? "flex" : "none"}} />
Following this approach, you wont need the extra if block
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
|
||
const ModalWrapper = props => { |
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.
Pls do add JsDoc to this JSX for the sake of documentation
|
||
.modal_body { | ||
padding: 25px; | ||
button { |
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 are you writing a style for button
that is not in you .modal_body`` div
. I think there is no need for this button style since there is no button in your JSX markup. Also a reuseable Button component will be created which will come with its own style
left: 0; | ||
height: 100vh; | ||
right: 0; | ||
background-color: rgba(54, 93, 220, 0.3); |
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 think for the sake of uniformity, pls use hex colour code, let reference the color from the theme color that is already created. But if the color we want to use is not there then we can add our hex color to the theme color in assets folder.
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.
how can i add opacity while using hex code?
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.
This is the hex code for it #365ddc4d
.
To reveal the hex code, you can click on the rgba text on the color palette when you hover on the rgba(54, 93, 220, 0.3)
on vscode
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.
Nice work so far, pls attend to my comments.
Also fix the two console.error message when you run yarn test.
under the test folder, pls create a component folder and move your Modal.test.js into the component folder. This will be good for files and folder organisation.
- create modal component - add styling to match mockups - write tests - set up jest [Delivers #168105593]
2348203
to
6172967
Compare
<div className={className}> | ||
<div className="modal_content"> | ||
<header className="modal_header"> | ||
<p>{heading}</p> |
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 think there will be need for className on the p tag, with props passed into it. This will create room for additional or optional style that is not handle here.
import PropTypes from 'prop-types'; | ||
|
||
const ModalWrapper = props => { | ||
const { heading, children, openModal, closeModal } = props; |
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 also think there will be no need for openModal props.
Doing this.state.openModal && <ModalWrapper />
will easily solve for the use of openModal.
This will save us extra lines of code.
state = {
openModal: false
}
showModal=()=>{
this.setSate({openModal: true})
}closeModal=()=>{
this.setSate({openModal: false})
}render(){
return(
this.state.openModal && <ModalWrapper
closeModal={this.closeModal}
heading="Create Event"
>
<div> children elements here </div>
</ModalWrapper>
);}
z-index: 2; | ||
|
||
.modal_content { | ||
background-color: #FFF; |
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.
pls reference the theme color under assets
What Does This PR Do?
This PR creates a reusable modal component.
Description Of Task To Be Completed
Any Background Context You Want To Provide?
N/A
How can this be manually tested?
openModal
,closeModal
andheading
props to the wrapper and then thechildren
through the props as well. Like show belowWhat are the relevant pivotal tracker stories?
#168105593
Screenshot(s)
N/A