Password comp #1316
Password comp #1316
Conversation
github-actions
bot
force-pushed
the
password-comp
branch
from
68a0423 to
4cd5d24
Compare
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #1316 +/- ##
============================================
+ Coverage 82.38% 82.50% +0.12%
- Complexity 946 959 +13
============================================
Files 105 106 +1
Lines 2430 2447 +17
Branches 332 334 +2
============================================
+ Hits 2002 2019 +17
Misses 260 260
Partials 168 168 ☔ View full report in Codecov by Sentry. |
devgurjar
force-pushed
the
password-comp
branch
from
4cd5d24 to
fdcdfb9
Compare
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
2 similar comments
Accessibility Violations Found
|
Accessibility Violations Found
|
Yes we have used new field type in this PR, please refer to this commit |
|
For opening and closing eye icon, we have changed code in theme |
Accessibility Violations Found
|
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
devgurjar
force-pushed
the
password-comp
branch
from
9432b5c to
f17a90a
Compare
github-actions
bot
force-pushed
the
password-comp
branch
from
f17a90a to
1943844
Compare
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
Accessibility Violations Found
|
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
Accessibility Violations Found
|
| * @since com.adobe.cq.forms.core.components.models.form 5.9.6 | ||
| */ | ||
| @ConsumerType | ||
| public interface Password extends Field, StringConstraint, NumberConstraint { |
There was a problem hiding this comment.
Password cannot have number constraint, please fix this. If you check the dialog, these number constraint properties are not present.
There was a problem hiding this comment.
I have removed numberConstraint
There was a problem hiding this comment.
This component was written initially by ACS back in 2023. Just trying to fix it for completion purpose.
| }, | ||
| "number-password" : { | ||
| "jcr:primaryType": "nt:unstructured", | ||
| "sling:resourceType" : "core/fd/components/form/textinput/v1/textinput", |
There was a problem hiding this comment.
Why is this text input ? Also there is nothing called number password
There was a problem hiding this comment.
This test was added to cover methods coming from numberContraint, as I have removed it so no longer required. Removed from json as well
| }, | ||
| ":type": "forms-components-examples/components/form/password" | ||
| }, | ||
| "number-password-exclusive" : { |
There was a problem hiding this comment.
Why do you need so many content ? Looks like it is copy pasted from text input, let's only have relevant bits here
There was a problem hiding this comment.
This test was added to cover methods coming from numberContraint, as I have removed it so no longer required. Removed from json as well
| @@ -0,0 +1,45 @@ | |||
| { | |||
There was a problem hiding this comment.
The pattern we follow with exporter is that it should match the corresponding content from test-content.json, in this case, exporter-password-customer.json should match password-customized test content structure, whereas you have not done that. How did the test case even pass ?
| "id": "password-477c777f4e", | ||
| "fieldType": "password", | ||
| "name": "password1708629052628", | ||
| "visible": true, |
There was a problem hiding this comment.
You've added these exporters, but there are no corresponding test cases (e.g., testJSONExport). Every file that's checked in should serve a specific purpose. Please ensure that everything is thoroughly verified before requesting a PR review.
There was a problem hiding this comment.
Added two new tests testExportedType and testJSONExport.
Accessibility Violations Found
|
Lighthouse scores (mobile)
|
Lighthouse scores (desktop)
|
Accessibility Violations Found
|
Accessibility Violations Found
|
1 similar comment
Accessibility Violations Found
|
Lighthouse scores (desktop)
|
Lighthouse scores (mobile)
|
Accessibility Violations Found
|
Accessibility Violations Found
|
devgurjar
force-pushed
the
password-comp
branch
from
4747ab8 to
a7b5e04
Compare
* GH actions * Dummy change * dummy * change * Fix error * Fix error * fix error * Fix error * fix regex * Fix code * dummy change * Dummy change * Fix errors
Updated Javascript code for toggle password visibility. Html code cleanups
…ct values on view
…and min values update
github-actions
bot
force-pushed
the
password-comp
branch
from
a7b5e04 to
4f278af
Compare
| } | ||
|
|
||
| @PostConstruct | ||
| private void initTextInput() { |
There was a problem hiding this comment.
This should be named as initPassword and not initTextInput
| * @since com.adobe.cq.forms.core.components.models.form 5.9.6 | ||
| */ | ||
| @ConsumerType | ||
| public interface Password extends Field, StringConstraint { |
There was a problem hiding this comment.
Please change the version and bump package-info file
| "readOnly": false, | ||
| "minLength": 5, | ||
| "maxLength": 10, | ||
| "maximum" : 16, |
There was a problem hiding this comment.
How can the exporter have maximum and minimum values when it's not part of the interface? Is this test even functioning properly? Please ensure such issues are verified before requesting a re-review.
| "id": "password-91417957c6", | ||
| "fieldType": "password", | ||
| "name": "password1732174214265", | ||
| "visible": true, |
There was a problem hiding this comment.
We don't add the default values for many properties to keep the JSON payload minimal. Its only added if its part of JCR, as per the following code, https://github.com/adobe/aem-core-forms-components/blob/master/bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/util/AbstractFormComponentImpl.java#L241
Please verify this
Also, if it isn't working for other components fix the same.
| @@ -0,0 +1,26 @@ | |||
| { | |||
There was a problem hiding this comment.
You’ve made the same mistake I highlighted in the previous review—these files are not even in used in the test (while it should have being)
Why did you request a re-review when the issue from my earlier comment has still not been addressed?
| cq:lastModified="{Date}2024-03-12T12:04:40.719+05:30" | ||
| cq:lastModifiedBy="admin" | ||
| cq:template="/conf/core-components-examples/settings/wcm/templates/content-page" | ||
| jcr:description="Text Input" |
There was a problem hiding this comment.
Why is the description Text Input when this is a password component ? Can you please fix these things at other places as well
| jcr:lastModifiedBy="admin" | ||
| jcr:primaryType="nt:unstructured" | ||
| sling:resourceType="core/wcm/components/text/v2/text" | ||
| text="<p>A text input (text box) component allows a user to enter and edit a single or multiple lines of text, depending on the type attribute of the input element. The text input component can be placed within a form and is usually labeled with a helpful text that easily identifies its purpose.</p><p>These text input fields can be used in a variety of ways, such as for collecting information like names, email addresses, phone numbers, and other types of text data. Additionally, the text input component can have additional functionality, styling, and accessibility features that can make it more user-friendly, efficient and accessible.</p>" |
There was a problem hiding this comment.
This description should be as per password component, it still refers text input. Please sync with CHL team for this
| By placing the class names `cmp-adaptiveform-password__label` and `cmp-adaptiveform-password__questionmark` within the `cmp-adaptiveform-password__label-container` class, you create a logical grouping of the label and question mark elements. This approach simplifies the process of maintaining a consistent styling for both elements. | ||
|
|
||
| ## Replace feature: | ||
| We support replace feature that allows replacing Password component to any of the below components: |
There was a problem hiding this comment.
Have you verified replace with all these components ?
| } | ||
|
|
||
| #togglePasswordType(){ | ||
| const widget = this.getWidget(); |
There was a problem hiding this comment.
Please fix the indentation
| * Html input type text | ||
| * @type {string} | ||
| */ | ||
| HTML_INPUT_TYPE_TEXT : "text", |
There was a problem hiding this comment.
Why is this constant in public namespace, this file is exposed to customers, I don't think we need such constants for something we don't own ? Please have it inline in place where it is required.
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: