Closed yuriyberezskyy closed 4 years ago
Also could you please replace "#YOUR-ISSUE-HERE" with the issue number so it will be close and maybe a screenshot of the popup
Thank you for all your comments and recommendations about my issue
On Tue, May 19, 2020 at 5:49 PM Carter Klein notifications@github.com wrote:
@SomeMoosery requested changes on this pull request.
Hopefully my change requests make sense.
We should definitely be using Redux to manage state between component, or I'm under the impression here that in lieu of that we can just move the [open, setOpen] state into the new dialog component and use it only there, as it seems like you're instantiating it in the survey component but only using it to pass into the dialog component as props anyway.
In client/src/components/survey/SurveyPage1.jsx https://github.com/COVID-19-electronic-health-system/Corona-tracker/pull/722#discussion_r427614417 :
@@ -106,6 +107,17 @@ const SurveyPage1 = props => { setSurveyPage(surveyPage + 1); };
- // HOOKS AND METHODS for PopUpContactAlert
Remove comment
In client/src/components/survey/PopUpContactAlert.jsx https://github.com/COVID-19-electronic-health-system/Corona-tracker/pull/722#discussion_r427615840 :
- CONTINUE
I personally think this should be moved back into the Survey page, and you should just handle displaying the dialog with setOpen there, since this is just a dialog component, not a button + dialog component and we won't need to pass around as many props.
In client/src/components/survey/SurveyPage1.jsx https://github.com/COVID-19-electronic-health-system/Corona-tracker/pull/722#discussion_r427616999 :
@@ -180,9 +192,19 @@ const SurveyPage1 = props => {
- + {dailyfeeling <= 2.0 && dailySymptomsFeeling <= 2.0 && dailyComparedToYesterday <= 2.0 ? ( + When passing state between components, it's best to use Redux. Although, I don't think many of these props need to be passed at all as mentioned in other comments.
In client/src/components/survey/SurveyPage1.jsx https://github.com/COVID-19-electronic-health-system/Corona-tracker/pull/722#discussion_r427617970 :
- const handleClickOpen = () => {
- setOpen(true);
- };
- const handleClose = () => {
- setOpen(false);
- };
I would suggest moving open (named more precisely) into Redux set, with an accompanying function to update that value. That way, we don't have pass around functions as props.
Even if not, we don't need separate handlers here, and can just call setOpen(true) or setOpen(false) directly.
In client/src/components/survey/PopUpContactAlert.jsx https://github.com/COVID-19-electronic-health-system/Corona-tracker/pull/722#discussion_r427618724 :
@@ -0,0 +1,54 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import Button from '@material-ui/core/Button'; +import Dialog from '@material-ui/core/Dialog'; +import DialogActions from '@material-ui/core/DialogActions'; +import DialogContent from '@material-ui/core/DialogContent'; +import DialogContentText from '@material-ui/core/DialogContentText'; +import DialogTitle from '@material-ui/core/DialogTitle'; + +const PopUpContactAlert = props => {
- const { open, handleClose, styleButton, submitSurveyPage1, handleClickOpen } = props;
We shouldn't be passing in any styling as props, either re-implement using makeStyles in this components or abstract into a helper file like we have for other buttons.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/COVID-19-electronic-health-system/Corona-tracker/pull/722#pullrequestreview-414841091, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM7RHFDUY67E4CHUK64SRGTRSL5G5ANCNFSM4NFJG4WA .
⚠️ IMPORTANT: Please do not create a Pull Request without creating an Issue first.
All changes need to be discussed before proceeding. Failure to do so may result in the pull request being rejected.
Before submitting a pull request, please be sure to review:
Please include the issue number the pull request fixes by replacing YOUR-ISSUE-HERE in the text below.
Fixes #707
Summary
I created an alert dialog when a person is in bad condition during going through the survey.
Details
In the folder Survey I created component PopUpContactAlert.jsx and pasted in SurveyPage1. I included Material-UI and some styles that we are using in the project. I compared conditions if person feels bad it's going to pop up alert window if it's not the person will continue the survey. I didn't know what to write in dialog so I wrote some information to my opinion.
Test Plan (required)
Final Checklist
package.json
?1.0.9
=>1.0.10
yarn
notnpm
? (important!)yarn lint
on the code?