Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: better assessment editor loading state #585

Merged
merged 10 commits into from
Sep 24, 2023
Merged

feat: better assessment editor loading state #585

merged 10 commits into from
Sep 24, 2023

Conversation

jfdoming
Copy link
Contributor

Notion ticket link

Improve save behaviour on admin assessments

Implementation description

  • Add full-page whiteout instead of loading spinner
  • Handle reload-prompt properly

Steps to test

  1. Open an assessment in the editor
  2. Click back and verify there is no prompt
  3. Open it again
  4. Edit something
  5. Click back and verify there is a prompt and hit cancel
  6. Click save
  7. Verify there is a full-screen whiteout box and you can't click any elements
  8. After saving finishes, click back
  9. Verify there is no prompt
  10. Repeat the edit-save cycle once more
  11. This time, instead of clicking back, edit a field again
  12. Click back and verify you see a prompt again

What should reviewers focus on?

Checklist

  • My PR name is descriptive and in imperative tense
  • My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
  • I have run the appropriate linter(s)
  • I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

@carissa-tang
Copy link
Collaborator

What about leaving an untouched question editor?

image

@github-actions
Copy link

github-actions bot commented Sep 15, 2023 •
edited
Loading

Visit the preview URL for this PR (updated for commit 60f7b4e):

https://jump-math-staging--pr585-julian-better-editor-y6659jm8.web.app

(expires Sat, 30 Sep 2023 20:18:28 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c42d8d0d853b05885664a2dd73f8245f4333ae51

@jfdoming
Copy link
Contributor Author

What about leaving an untouched question editor?

image

hmm, I felt like that wasn't a concern of this ticket, but I'll fix it for this PR, sure

carissa-tang
carissa-tang reviewed Sep 15, 2023
View reviewed changes
Copy link
Collaborator

@carissa-tang carissa-tang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated to this PR but save error shouldn't show here

image

also when i create a new test, there shouldn't be the warning (+ should we also not redirect users to assessments page after create?)

image

@jfdoming
Add full-page whiteout instead of loading spinner
c2262e0 
Also don't redirect away from the assessment-editor after saving.
@jfdoming
Handle reload-prompt properly
ad38974 
Now that we stay on the editor page sometimes after submission, we need
to fix the handling of the reload prompt (otherwise, it will never
display after the form is submitted for the first time). To do this, we
check if the form state has been touched (dirtied), and only allow
reloading if it has not. We also have to reset the form default values
as this is how `rhf` determines whether the form is dirty.
@jfdoming
Copy link
Contributor Author

What about leaving an untouched question editor?
image

hmm, I felt like that wasn't a concern of this ticket, but I'll fix it for this PR, sure

actually looks like this will be fairly complex, I'd like to make this a separate ticket

@jfdoming
Copy link
Contributor Author

separate ticket: https://www.notion.so/uwblueprintexecs/Fix-reload-prompt-for-question-editor-dd2c97a68b2a44de91462b8b411569e9

@jfdoming jfdoming force-pushed the julian/better-editor-loading-state branch from b9583b6 to 6d036ec Compare September 17, 2023 19:08
carissa-tang
carissa-tang reviewed Sep 17, 2023
View reviewed changes
Copy link
Collaborator

@carissa-tang carissa-tang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now when i click save without filling out everything, i see "Assessment saved" lol

image

and just to confirm, we're intentionally leaving the window pop up after clicking Create Assessment > Back?

frontend/src/components/admin/assessment-creation/AssessmentEditorHeader.tsx Show resolved Hide resolved
@carissa-tang carissa-tang added question Further information is requested ready to merge and removed ready for review labels Sep 23, 2023
@carissa-tang carissa-tang added requested changes question Further information is requested and removed question Further information is requested ready to merge labels Sep 23, 2023
@jfdoming jfdoming force-pushed the staging branch 8 times, most recently from 8f3a454 to 52da9eb Compare September 24, 2023 08:58
@carissa-tang carissa-tang added ready to merge and removed question Further information is requested requested changes labels Sep 24, 2023
@jfdoming jfdoming merged commit eae1974 into staging Sep 24, 2023
7 checks passed
@jfdoming jfdoming deleted the julian/better-editor-loading-state branch September 24, 2023 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carissa-tang carissa-tang carissa-tang approved these changes

@joyce-shi joyce-shi Awaiting requested review from joyce-shi

Assignees
No one assigned
Labels
ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jfdoming @carissa-tang