Conversation
There was a problem hiding this comment.
I noticed that ProjectViewerModal and SubmissionViewerModal have been updated to use the new ModalWrapper library. I think it would be best if the ActionModal is also updated to use this wrapper (for consistency's sake).
ui/src/css/components/modal.css
Outdated
| } | ||
|
|
||
| .custom-confirm-overlay { | ||
| z-index: 99999 !important; |
There was a problem hiding this comment.
I would reconsider having such a high z-index. In the future, people might want to overlay over top this component (as unlikely as it may be) and would need a ridiculously high z-index for it.
ui/src/css/components/modal.css
Outdated
| top: 50% !important; | ||
| left: 50% !important; | ||
| transform: translate(-50%, -50%) !important; | ||
| z-index: 100000 !important; |
There was a problem hiding this comment.
similar idea here regarding z-index, my guess is its even higher than the last since it needs to go on top of it.
llf5185
left a comment
llf5185
left a comment
There was a problem hiding this comment.
Additionally, the modal used in ProposalPage.js has not been updated to match this new standard for modals.
This was what I was able to find so far, but there are likely more issues with this pull request I overlooked.
Leaving a note here that react-confirm-alert must be installed in the ui folder when updating the server to include these new features.
|
|
||
| /* Fix for sticky modals like ActionModal */ | ||
| .ui.modal.sticky { | ||
| position: fixed !important; |
There was a problem hiding this comment.
Be careful about using !important so frequently. If you need to use !important, justify its use with comments.
| // const [due, setDue] = useState(); | ||
| // const [late, setLate] = useState(false); | ||
| // const [day, setDay] = useState(0); | ||
| const { user } = useContext(UserContext); |
There was a problem hiding this comment.
Naming this variable user when there is a property called user may be confusing.
| const { user } = useContext(UserContext); | |
| const { userContext } = useContext(UserContext); |
| )} | ||
| </div> | ||
| } | ||
| title={`Time Submission For ${props.user}`} |
There was a problem hiding this comment.
The value passed from ProjectTIme.js to the IndividualTimeModal component for props.user is a JSX element, thus this text will render as:
Time Submission For [object Object]
| @@ -714,14 +771,64 @@ export default function DatabaseTableEditor(props) { | |||
| closeOnDimmerClick={false} | |||
| closeOnEscape={false} | |||
There was a problem hiding this comment.
To maximize the amount of page interaction that can be done with just the keyboard and for the sake of consistency, all modals should close on escape or have a comment stating why they shouldn't.
There was a problem hiding this comment.
The widths of the new modals are based on the amount of content, which causes their widths to be inconsistent with each other and leads to the strange formatting shown in the below image.
This example uses the single character 'a' in each required field and thus causes an unexpectedly thin modal.
There was a problem hiding this comment.
Confirming the creation of a sponsor note produces an unsaved changes warning.
The screenshot below shows the warning given after pressing submit on a sponsor note.
Below screenshot shows what happens after you press "Discard Changes".
Below screenshot shows what happens after pressing "Keep Editing". In this case, the sponsor note is still created, but after closing the success modal, the sponsor note editor is left open.
5 participants
Changes in this PR