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.

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

MDT-2029: Loading Spinner for "Current Banner" Section (Admin Only) #4330

Merged
merged 6 commits into from
Oct 12, 2022

Conversation

karla-vm
Copy link
Collaborator

Description

Closes MDCT-2029.

The current /admin banner page displays “There is no current banner” if the app does not have any fetched admin banner, but if you're still waiting for that fetch to complete, the message shouldn't be displaying and should be a Loading Spinner instead.

How to test

./dev local

  • Log in as adminuser@test.com
  • Navigate to /admin
  • Create a new banner
  • Refresh the page
  • Delete that banner
  • Refresh the page

See the spinner spinnin'

Changed Dependencies

N/A

Code author checklist

  • I have performed a self-review of my code
  • I have added thorough tests
  • I have added analytics, if necessary
  • I have updated the documentation, if necessary

Reviewer checklist (two different people)

  • I have done the deep review and verified the items in the above checklist are g2g
  • I have done the lgtm review

@ntsummers1
Copy link
Contributor

This works, but it leaves the spinner up after the result has come back as shown in this clip. Also, the spinners contrast is a little faint, would it be better to have it be a little darker here?

Screen.Recording.2022-10-11.at.3.07.08.PM.mov

Copy link
Contributor

@gmrabian gmrabian left a comment

Choose a reason for hiding this comment

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

I'm not seeing the issue Nick called out

@karla-vm
Copy link
Collaborator Author

This works, but it leaves the spinner up after the result has come back as shown in this clip. Also, the spinners contrast is a little faint, would it be better to have it be a little darker here?
Screen.Recording.2022-10-11.at.3.07.08.PM.mov

This should be fixed now 🎉 can you give it another go?

Copy link
Contributor

@ntsummers1 ntsummers1 left a comment

Choose a reason for hiding this comment

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

Looks great!

@karla-vm karla-vm merged commit 6f730be into main Oct 12, 2022
@karla-vm karla-vm deleted the mdct-2029 branch October 12, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants