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

Add spinner to the Application Form terms page upon submit #1225

Merged
merged 4 commits into from
May 18, 2021
Merged

Add spinner to the Application Form terms page upon submit #1225

merged 4 commits into from
May 18, 2021

Conversation

jaredcwhite
Copy link
Collaborator

@jaredcwhite jaredcwhite commented May 11, 2021 •
edited
Loading

Issue

Addresses #1193

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

The submit button on the Terms page for the Application was allowing multiple clicks before the form was submitted. This PR uses the "loading" state of the button component to provide visual feedback as well as preventing additional clicks.

In addition, I:

  • Refactored the loading overlay components to use the spinner in the icons SVG
  • Changed the spinner SVG so it uses CSS animation (apparently SMIL is deprecated, also buggy in Safari)
  • Equalized the spinner icon SVG across our three packages (previously the public site was out of date)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

I manually tested the Terms page of the Application Form, and wrote an example in Storybook for LoadingOverlay and Button with the loading state.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers

@jaredcwhite
Add spinner to the Application Form terms page on submit
f9fbde3 
* Refactor the loading overlay to use the icons SVG
* Use CSS animation for spinner
* Equalize the spinner icon across three packages
@jaredcwhite
Better loading overlay story layout
bfadda4
@jaredcwhite
Fix test for LoadingOverlay
31e553f
@exygy-dev exygy-dev temporarily deployed to bloom-refere-add-loadin-px37vl May 11, 2021 17:22 Inactive
@netlify
Copy link

netlify bot commented May 11, 2021 •
edited
Loading

Deploy preview for clever-edison-cd22c1 ready!

Built with commit 0508f96

https://deploy-preview-1225--clever-edison-cd22c1.netlify.app

@emilyjablonski
Copy link
Collaborator

emilyjablonski commented May 12, 2021 •
edited
Loading

The spinner refactor looks great! I'm a little bit unclear about which part of these changes prevents the user from clicking the button more than once and spamming the submit function. From what I can tell a single click starts the spinner but doesn't prevent multiple submits? Definitely could be missing something but I think we need a (if not submitting already) at the top of onSubmit?

Update: Found it! It was hidden within Button :)

@jaredcwhite
Copy link
Collaborator Author

@dominikx96 Let me know if this looks good to you, then I can merge. Thanks!

dominikx96
dominikx96 approved these changes May 17, 2021
View reviewed changes
Copy link
Collaborator

@dominikx96 dominikx96 left a comment

Choose a reason for hiding this comment

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

@jaredcwhite LGTM! 👍

sites/partners/public/images/icons.svg
<circle cx="50" cy="50" fill="none" stroke="currentColor" stroke-width="9" r="32" stroke-dasharray="150.79644737231007 52.26548245743669">
<animateTransform attributeName="transform" type="rotate" repeatCount="indefinite" dur="1s" values="0 50 50;360 50 50" keyTimes="0;1"></animateTransform>
</circle>
<circle cx="50" cy="50" fill="none" stroke="currentColor" stroke-width="9" r="32" stroke-dasharray="150.79644737231007 52.26548245743669"></circle>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably conflict with: #1230

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I'll suggest a change to 1230 to incorporate this.

@jaredcwhite jaredcwhite merged commit 0568ef8 into master May 18, 2021
jaredcwhite added a commit that referenced this pull request May 18, 2021
@jaredcwhite
Update changelog with #1225
1ba35b9
@jaredcwhite jaredcwhite mentioned this pull request May 18, 2021
Merged
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilyjablonski emilyjablonski emilyjablonski left review comments

@dominikx96 dominikx96 dominikx96 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jaredcwhite @emilyjablonski @dominikx96 @exygy-dev