-
Notifications
You must be signed in to change notification settings - Fork 58
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
FORMS-9362: loading icon behind fields #880
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #880 +/- ##
=========================================
Coverage 80.07% 80.07%
Complexity 714 714
=========================================
Files 87 87
Lines 2023 2023
Branches 269 269
=========================================
Hits 1620 1620
Misses 254 254
Partials 149 149 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Test cases were already present in command.js therefore, I have updated the test case according to the latest code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test cases were already present in command.js therefore, I have updated the test case according to the latest code.
.../apps/core/fd/components/form/container/v2/container/clientlibs/site/js/formcontainerview.js
Outdated
Show resolved
Hide resolved
.../apps/core/fd/components/form/container/v2/container/clientlibs/site/js/formcontainerview.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check changes
68e8f5d
to
6454483
Compare
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
@@ -63,14 +63,20 @@ | |||
const startTime = new Date().getTime(); | |||
let elements = document.querySelectorAll(FormContainerV2.selectors.self); | |||
for (let i = 0; i < elements.length; i++) { | |||
elements[i].classList.add(FormContainerV2.loadingClass); | |||
let loaderToAdd = document.querySelector("[data-cmp-adaptiveform-container-loader='"+ elements[i].id + "']"); | |||
if(loaderToAdd){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix formatting here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write a test case for the fix
8aab3ae
to
357ff87
Compare
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
* FORMS-9362: loading icon behind fields * FORMS-9362 : null check added as per comments * FORMS-9362 : changes due to review comments * FORMS-9362 : formatting the changes
Description
As discussed with Rishi, the sibling div should have the selector as the ID of form and selecting the div with the same and adding the loading class on it later removing as well with the same steps.
However the test cases were already written in command.js file therefore, I have made the corrections in the same for the updated logic.
the new updates has been added to the readme file as well.
Related Issue
FORMS-9362: Loading Icon Behind Fields
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: