hcs-t4sg / questable

questable.vercel.app
2 stars 0 forks source link

Form and modal standardization #70

Closed IsoPhoenix closed 1 year ago

IsoPhoenix commented 1 year ago

Overview

Right now, some of Questable's modals and forms (ex: creating a task) are not implemented properly to work with keyboard inputs (ex: submitting on Enter). Likewise, some of our modals don't work with keyboard inputs either (ex: exiting upon escape). We should make small rewrites to the forms/modals that don't work, using MUI's form/dialog logic, to ensure that they're responsive in the correct way.

Form fixes

Forms should submit upon hitting enter. You should be able to do this by wrapping the form in an MUI FormControl and having the submit button be type='submit' with no onClick. See more info here.

For situations where an MUI FormControl is not available, you can use a regular HTML <form> component. An example of this situation is the chatbox in ForumPostView.tsx.

Modal fixes

Modals should close upon hitting Escape. This should be taken care of if the modal is implemented using MUI Dialog component (reference here).

Note that a lot of the modals that don't work right now are implemented as TeacherModalStyled, which is a custom component in TaskModalStyles.tsx. This is because TeacherModalStyled expands the MUI Modal component (a lower-level wrapper utilized by the Dialog component) and not the Dialog component itself. The best fix for all our modals that utilize TeacherModalStyled is to change TeacherModalStyled to use styled() on a Dialog component instead. However, at first try this causes some layout issues with the modals that we will need to fix.

jaydenpersonnat commented 1 year ago

For the form, I implemented the fixes described above. The one thing to note though is that the user can only currently submit a form via clicking enter while their cursor is on a TextField, as hitting 'enter' does not trigger an event for the modal itself. To resolve this, we could wrap the form around the entire modal and add an event handler to the modal that's listening for when 'enter' key is pressed, though this is a bit problematic as clicking 'enter' in the text editor used for descriptions would submit the form, rather than creating a new line (similar to how Slack works). I can probably edit the default settings of the text editor to avoid this but wanted to make sure this is something we should try to resolve.

As for the Modal fixes, they'll close upon a user hitting esc as well as when a user clicks in the background. I was able to fix this issue still expanding the MUI Modal component. However, should we still refactor to use Dialog instead?

All the changes I've made so far are in the branch form-modal-standardization.

jaydenpersonnat commented 1 year ago

Also, I noticed while fixing the bugs above that editing the Max Completions field for repeatable does not currently update the database, and the logic for that is editing out. Is this something we would like to fix?

IsoPhoenix commented 1 year ago

I think the way the form is implemented right now for task creation is fine! Pressing enter when not editing the description will submit whereas pressing enter inside the description editing does not submit, which is what I expect.

Sorry for a bit of misdirection earlier, but I think you could also replace the standard HTML

component with an MUI instead, and it should accept the onSubmit and other props. Just an MUI way of doing things that gives access to MUI styling/accessibility as well.

I think we should still refactor to use Dialog in this case; MUI might be doing some additional accessibility stuff under the hood with dialog that’s not easily implementable with Modal, and since our code will be open source it’s best to follow MUI conventions.

Lmk if you have any more questions and thanks so much!!

Matthew Su Harvard College Class of 2024 A.B. Candidate in Chemistry and Computer Science On Apr 22, 2023 at 6:47 PM -0400, Jayden Personnat @.***>, wrote:

For the form, I implemented the fixes described above. The one thing to note though is that the user can only currently submit a form via clicking enter while their cursor is on a TextField, as hitting 'enter' does not trigger an event for the modal itself. To resolve this, we could wrap the form around the entire modal and add an event handler to the modal that's listening for when 'enter' key is pressed, though this is a bit problematic as clicking 'enter' in the text editor used for descriptions would submit the form, rather than creating a new line (similar to how Slack works). I can probably edit the default settings of the text editor to avoid this but wanted to make sure this is something we should try to resolve. As for the Modal fixes, they'll close upon a user hitting esc as well as when a user clicks in the background. I was able to fix this issue still expanding the MUI Modal component. However, should we still refactor to use Dialog instead? All the changes I've made so far are in the branch form-modal-standardization. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

IsoPhoenix commented 1 year ago

I don’t think it’s a priority right now because we’d have to put a lot of thought into making sure changing max completions works with repeatable completion and refreshing logic. A teacher can always delete the repeatable and make a new one so I don’t think it’s a huge deal right now

Matthew Su Harvard College Class of 2024 A.B. Candidate in Chemistry and Computer Science On Apr 22, 2023 at 6:51 PM -0400, Jayden Personnat @.***>, wrote:

Also, I noticed while fixing the bugs above that editing the Max Completions field for repeatable does not currently update the database, and the logic for that is editing out. Is this something we would like to fix? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>