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

feat: add loading circles to modals #21

Merged
merged 23 commits into from
Jan 10, 2024
Merged

Conversation

ilee2u
Copy link
Member

@ilee2u ilee2u commented Dec 12, 2023

  • reset, reject, and verify buttons all have loading circles now
  • also, renamed the components to modals since that's what they are

Issue: https://2u-internal.atlassian.net/browse/COSMO-121

Demo:

Screen.Recording.2023-12-13.at.10.13.30.AM.mov

Error Button:

image

- reset, reject, and verify buttons all have loading circles now
- also, renamed the components to modals since that's what they are
@ilee2u ilee2u marked this pull request as draft December 12, 2023 19:39
@ilee2u ilee2u marked this pull request as ready for review December 12, 2023 19:41
@ilee2u ilee2u closed this Dec 12, 2023
@ilee2u ilee2u reopened this Dec 12, 2023
@ilee2u
Copy link
Member Author

ilee2u commented Dec 12, 2023

Whoops I hit the close button by accident, ignore that.

Also this PR does still need test fixes.

@ilee2u
Copy link
Member Author

ilee2u commented Dec 13, 2023

Planned next steps:

  • Disable the "Verify" button when the "Reject" button is clicked
  • Add tests for this component

@ilee2u ilee2u force-pushed the ilee2u/buttons-add-loading branch from 7ab9d79 to e75c81a Compare December 14, 2023 21:56
@ilee2u
Copy link
Member Author

ilee2u commented Dec 20, 2023

NOTE: Apparently linking the "completed" status of the request to the button's state makes the button get stuck in the "completed" state after fulfilling one request, so I'm going to remove the completed state but keep the pending one.

image

@ilee2u
Copy link
Member Author

ilee2u commented Dec 20, 2023

Added a demo of the error button to the original post, which does the same thing as the "completed" button where it stays in the modal even if you open a new one, though I suppose one would refresh the page on an error anyhow so I will keep this status

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (9ce2fce) 75.75% compared to head (9b5bb84) 76.75%.

Files Patch % Lines
src/data/redux/requests/selectors.js 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #21      +/-   ##
==========================================
+ Coverage   75.75%   76.75%   +0.99%     
==========================================
  Files          22       22              
  Lines         297      314      +17     
  Branches       31       34       +3     
==========================================
+ Hits          225      241      +16     
- Misses         69       70       +1     
  Partials        3        3              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ilee2u ilee2u force-pushed the ilee2u/buttons-add-loading branch from 190704e to 46bfb73 Compare January 8, 2024 17:14
src/pages/ExamsPage/hooks.js Outdated Show resolved Hide resolved
src/pages/ExamsPage/hooks.test.js Outdated Show resolved Hide resolved
src/testUtils.js Outdated Show resolved Hide resolved
import * as testUtils from '../../../testUtils';

import * as hooks from '../hooks';

jest.mock('../data/api', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

we removed the lines assigning a response to this mock so I don't think this is doing anything

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed 👍

Copy link
Contributor

@zacharis278 zacharis278 left a comment

Choose a reason for hiding this comment

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

one missed debugging comment but otherwise looking good!

@@ -54,6 +54,8 @@ export const useDeleteExamAttempt = () => {
};

export const useModifyExamAttempt = () => {
// something's up with useMakeNetworkRequest here?
// ok it says line 1473... but there's only like 200 in this file
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: extra comment

@ilee2u ilee2u merged commit 2ea6ad1 into main Jan 10, 2024
3 checks passed
@ilee2u ilee2u deleted the ilee2u/buttons-add-loading branch January 10, 2024 14:43
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