implement and style modal #1552
Conversation
|
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes. |
| <Typography sx={{textAlign: 'center'}} id="modal-modal-title" variant="h4" component="h4"> | ||
| Wait! You made some changes. | ||
| </Typography> | ||
| <Typography id="modal-modal-description" sx={{ mt: 2 }}> |
There was a problem hiding this comment.
@spiteless All changes are made and ready for review
There was a problem hiding this comment.
There's an error somewhere with the icon imports:
[0] Mongoose connected from runServer() and is listening on 4000
[1] Browserslist: caniuse-lite is outdated. Please run:
[1] npx update-browserslist-db@latest
[1] Why you should do it regularly: https://github.com/browserslist/update-db#readme
[1] Failed to compile.
[1]
[1] ./src/components/ChangesModal.js
[1] Module not found: Can't resolve '@mui/icons-material/WarningAmberRounded' in '/Users/trilliumsmith/code/VRMS/VRMS3/client/src/components'
[1] Compiling...
[1] Failed to compile.
[1]
[1] ./src/components/ChangesModal.jsx
[1] Module not found: Can't resolve '@mui/icons-material/WarningAmberRounded' in '/Users/trilliumsmith/code/VRMS/VRMS3/client/src/components'Not sure what's causing that, it was happening too with the react icon you used earlier and I just added it to the package.json to test.
This is on top of navigating to ./client/ and running yarn install again
client/src/components/ProjectForm.js
Outdated
| variant="contained" | ||
| cursor="pointer" | ||
| onClick={handleOpen} |
There was a problem hiding this comment.
The open/close logic needs a bit more work.
Right the modal is opens warning of changes even if no changes were made.
Modal should open if:
- Changes were made to the form the modal is referencing and
- The user tries to navigate away from the current page
- The user presses the Close button
The Modal should not open if:
- The user tries to navigate away from then page and no changes were made
- The user presses the close button and no changes were made
Some sort of ModalShouldTrigger prop might help so that the parent component can determine when the modal should mess with user navigation.
client/src/pages/UserAdmin.js
Outdated
| const backToSearch = () => { | ||
| setUserToEdit({}); | ||
| }; | ||
| // const backToSearch = () => { |
There was a problem hiding this comment.
Pretty sure changes to this file were a mistake, please undo changes to UserAdmin.js.
package.json
Outdated
| @@ -20,7 +20,8 @@ | |||
| "concurrently": "^5.3.0", | |||
| "cross-env": "^7.0.2", | |||
| "cross-var": "^1.1.0", | |||
| "dotenv-cli": "^3.2.0" | |||
| "dotenv-cli": "^3.2.0", | |||
| "react-icons": "^4.12.0" | |||
There was a problem hiding this comment.
I'd prefer if we kept our icon usage in MUI's icon library if possible so we don't have to take on other dependencies.
Alternate icon suggestion:
https://mui.com/material-ui/material-icons/?query=warn&theme=Rounded
| @@ -17,6 +17,7 @@ | |||
| "test:all": "cross-env NODE_ENV=test npm run test:client && npm run test:client-mvp && npm run test:backend && npm run test:cy" | |||
| }, | |||
| "dependencies": { | |||
| "@mui/icons-material": "^5.14.19", | |||
There was a problem hiding this comment.
Lol, I thought we would not have to add another dependency. At least sits in the MUI ecosystem
| onClose={handleClose} | ||
| destination={'/projects'} | ||
| aria-labelledby="modal-modal-title" | ||
| aria-describedby="modal-modal-description" |
There was a problem hiding this comment.
Please remove these two props, they're already added to the model
| variant="contained" | ||
| cursor="pointer" | ||
| onClick={Object.keys(dirtyFields).length > 0 ? handleOpen: checkFields} |
There was a problem hiding this comment.
Suggests renaming checkFields to something related to navigation. It makes sense after looking at it for a minute, but if it was something like ? openModel : continueNavigation it might be easier for the next person who has to maintain it.
There was a problem hiding this comment.
Which is very weird because your PR adds "@mui/icons-material": "^5.14.19", to ./client/package.json
| <Typography sx={{textAlign: 'center'}} id="modal-modal-title" variant="h4" component="h4"> | ||
| Wait! You made some changes. | ||
| </Typography> | ||
| <Typography id="modal-modal-description" sx={{ mt: 2 }}> |
There was a problem hiding this comment.
There's an error somewhere with the icon imports:
[0] Mongoose connected from runServer() and is listening on 4000
[1] Browserslist: caniuse-lite is outdated. Please run:
[1] npx update-browserslist-db@latest
[1] Why you should do it regularly: https://github.com/browserslist/update-db#readme
[1] Failed to compile.
[1]
[1] ./src/components/ChangesModal.js
[1] Module not found: Can't resolve '@mui/icons-material/WarningAmberRounded' in '/Users/trilliumsmith/code/VRMS/VRMS3/client/src/components'
[1] Compiling...
[1] Failed to compile.
[1]
[1] ./src/components/ChangesModal.jsx
[1] Module not found: Can't resolve '@mui/icons-material/WarningAmberRounded' in '/Users/trilliumsmith/code/VRMS/VRMS3/client/src/components'Not sure what's causing that, it was happening too with the react icon you used earlier and I just added it to the package.json to test.
This is on top of navigating to ./client/ and running yarn install again
Fixes #1391
What changes did you make and why did you make them ?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied