chore: fix-form-fields-tests#162
Conversation
|
|
View your CI Pipeline Execution ↗ for commit 64c4723.
☁️ Nx Cloud last updated this comment at |
|
Deployed 90c1fd1 to https://ForgeRock.github.io/ping-javascript-sdk/pr-162/90c1fd131cf7db8619611290ed24b7ddf8240376 branch gh-pages in ForgeRock/ping-javascript-sdk |
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (20.00%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #162 +/- ##
==========================================
+ Coverage 44.92% 50.59% +5.66%
==========================================
Files 33 24 -9
Lines 1538 1354 -184
Branches 186 177 -9
==========================================
- Hits 691 685 -6
+ Misses 847 669 -178
🚀 New features to boost your workflow:
|
| ['dropdown-field-key']: '', | ||
| ['radio-group-key']: '', | ||
| 'combobox-field-key': ['option1 value', 'option2 value'], | ||
| 'checkbox-field-key': [], |
There was a problem hiding this comment.
i'm not rendering this because the data we have ends up with the same exact text / label for this and radio
and then we have to getByAll and index through it I think or filter them. we can do it I just chose not to.
There was a problem hiding this comment.
These components i "wrote" with AI, so it did much of the heavy lifting. so excuse the blocks of code.
cerebrl
left a comment
cerebrl
left a comment
There was a problem hiding this comment.
I think much of this code written by AI needs to be deeply refactored. It seems to have mimicked how many people try to re-engineer native elements for various reasons that are usually not good ideas.
| // If this checkbox is being checked | ||
| if (target.checked) { | ||
| // Uncheck the previously selected checkbox if there was one | ||
| if (selectedCheckbox && selectedCheckbox !== target) { | ||
| selectedCheckbox.checked = false; | ||
| } | ||
|
|
||
| // Update the selected checkbox reference | ||
| selectedCheckbox = target; | ||
|
|
||
| console.log(event.target); | ||
| // Call the updater with the selected value | ||
| updater(target.value); | ||
| } else { | ||
| // If the user is trying to uncheck the selected checkbox, | ||
| // prevent that to maintain a selected state | ||
| target.checked = true; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I'm a bit confused by this code. It seems there's a belief that this should act like a radio group? If so, that is incorrect as "checkbox", in this case is "multi-value".
| }; | ||
|
|
||
| // Populate dropdown with options | ||
| collector.output.options.forEach((option) => { |
There was a problem hiding this comment.
Rather than all of this, I'd just recommend using the native <select multiple> option: https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/multiple.
| ) { | ||
| dropdownComponent(formEl, collector, davinciClient.update(collector)); | ||
| } else if (collector.type === 'SingleSelectCollector' && collector.input.type === 'RADIO') { | ||
| checkboxComponent(formEl, collector, davinciClient.update(collector)); |
There was a problem hiding this comment.
If the type is RADIO, why are we using "checkbox"?
| await page.getByRole('checkbox', { name: 'option2 label' }).check(); | ||
| await page.getByRole('checkbox', { name: 'option3 label' }).check(); | ||
| await page.getByRole('checkbox', { name: 'option2 label' }).check(); | ||
| await page.getByRole('button', { name: 'Select options ▼' }).click(); | ||
| await page.locator('[id="combobox-field-key-option-option1\\ value"]').check(); | ||
| await page.locator('[id="combobox-field-key-option-option2\\ value"]').check(); | ||
| await page.locator('[id="combobox-field-key-option-option3\\ value"]').check(); | ||
| await page.locator('[id="combobox-field-key-option-option1\\ value"]').uncheck(); | ||
| await page.locator('[id="combobox-field-key-option-option3\\ value"]').uncheck(); | ||
| await page.locator('[id="combobox-field-key-option-option1\\ value"]').check(); | ||
| await page.getByRole('button', { name: 'Apply' }).click(); |
There was a problem hiding this comment.
I think if we switch to a native element, that would clean this up quite a bit.
cerebrl
force-pushed
the
form-fields-tests
branch
2 times, most recently
from
cef7fd7 to
1ef084c
Compare
cerebrl
force-pushed
the
form-fields-tests
branch
from
1ef084c to
64c4723
Compare
3 participants
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-3657?atlOrigin=eyJpIjoiODI5YWZmZTc5MGZlNDFiOGFkMDEyMGMzY2QwNDBlNDYiLCJwIjoiaiJ9
Description
Fixes the current form-fields tests. adds the required configuration to the server-config file.
We may want to adjust this in the tenant (which requires changes here) so that its known this is a test configuration in the tenant.
This test really just ensures that when we submit the required form fields, the request is sent and matches the correct schema when we send it.