V2 version of Title #1292

pavi41 · 2024-06-27T03:36:56Z

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

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 change)

Checklist:

I have signed the Adobe Open Source CLA.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes and the overall coverage did not decrease.
All unit tests pass on CircleCi.
I ran all tests locally and they pass.

codecov · 2024-06-27T04:28:38Z

Codecov Report

Attention: Patch coverage is 82.35294% with 6 lines in your changes missing coverage. Please review.

Project coverage is 81.68%. Comparing base (2a3af21) to head (c65f065).

Files	Patch %	Lines
...omponents/internal/models/v2/form/TitleImplV2.java	80.64%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##                dev    #1292      +/-   ##
============================================
+ Coverage     81.67%   81.68%   +0.01%     
- Complexity      853      873      +20     
============================================
  Files            97       99       +2     
  Lines          2259     2293      +34     
  Branches        304      312       +8     
============================================
+ Hits           1845     1873      +28     
- Misses          255      257       +2     
- Partials        159      163       +4

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

adobe-bot · 2024-07-02T06:07:38Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	92	96	100	75

adobe-bot · 2024-07-02T06:07:38Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	100	75

adobe-bot · 2024-07-02T06:07:49Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-02T06:09:34Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-02T06:10:06Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-02T08:51:25Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	100	75

adobe-bot · 2024-07-02T08:51:25Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	92	96	100	75

adobe-bot · 2024-07-02T08:51:36Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-02T08:53:24Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-02T08:54:57Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

rismehta

Lots of issues. Please check comments, I will do another review in the next iteration

rismehta · 2024-07-03T07:56:52Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/FormTitle.java

+     * Retrieves the text value to be displayed.
+     *
+     * @return the text value to be displayed, or {@code null} if no value can be returned
+     * @since com.adobe.cq.forms.core.components.models.form 0.0.1;


Please fix the java docs

rismehta · 2024-07-03T07:57:14Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/FormTitle.java

+// import com.adobe.cq.forms.core.components.models.form.BaseConstraint.Type;
+
+@ConsumerType
+public interface FormTitle extends FormComponent {


Please mention in comments on why this was copied from WCM

rismehta · 2024-07-03T07:57:26Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/FormTitle.java

+// import com.adobe.cq.forms.core.components.models.form.BaseConstraint.Type;
+
+@ConsumerType
+public interface FormTitle extends FormComponent {


Please add java doc for all functions, interfaces

Implemented!

rismehta · 2024-07-03T07:58:09Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/FormTitle.java

+     * @return the element type
+     * @since com.adobe.cq.forms.core.components.models.form 0.0.1;
+     */
+    default String getFormat() {


We should rename this to getHTMLElementType rather than format

Implemented!

rismehta · 2024-07-03T07:58:24Z

bundles/af-core/src/main/java/com/adobe/cq/forms/core/components/models/form/FormTitle.java

+     * Returns the HTML element type (h1-h6) used for the markup.
+     *
+     * @return the element type
+     * @since com.adobe.cq.forms.core.components.models.form 0.0.1;


Please bump package-info file and update the version here accordingly

rismehta · 2024-07-03T08:00:27Z

...re/src/main/java/com/adobe/cq/forms/core/components/internal/models/v2/form/TitleImplV2.java

+
+    @Override
+    public String getName() {
+        return null;


Name is mandatory if a component is a form component, please check other implementation. We should also expose this in dialog

Implemented!

Why the title component should have a name. We have the chance to enable unnamed fields now, though we do not need to expose it but we can fix in our code wherever we have a hard restriction on name property

Why the title component should have a name. We have the chance to enable unnamed fields now, though we do not need to expose it but we can fix in our code wherever we have a hard restriction on name property

In the core components, the name is mandatory as per the initial design, even though af-core does not enforce this requirement. While we can remove the name from the title component, doing so would prevent dynamic changes to the title component's text. Currently, we don't have a use case for this dynamic capability, so I'm okay with not implementing it.

rismehta · 2024-07-03T08:00:55Z

...re/src/main/java/com/adobe/cq/forms/core/components/internal/models/v2/form/TitleImplV2.java

+    @ScriptVariable(injectionStrategy = InjectionStrategy.OPTIONAL)
+    @JsonIgnore
+    @Nullable
+    private Style currentStyle;


Add current style in AbstractFormComponent

Implemented!

rismehta · 2024-07-03T08:01:08Z

...re/src/main/java/com/adobe/cq/forms/core/components/internal/models/v2/form/TitleImplV2.java

+    private Resource resource;
+
+    @ScriptVariable(injectionStrategy = InjectionStrategy.OPTIONAL)
+    @JsonIgnore


JsonIgnore should be part of getters, you don't need it here

rismehta · 2024-07-03T08:01:31Z

...re/src/main/java/com/adobe/cq/forms/core/components/internal/models/v2/form/TitleImplV2.java

+
+    @ValueMapValue(injectionStrategy = InjectionStrategy.OPTIONAL, name = "format")
+    @Nullable
+    private String format;


You should only inject things required for title here, other base properties/method should derive from parent class

rismehta · 2024-07-03T08:01:56Z

...re/src/main/java/com/adobe/cq/forms/core/components/internal/models/v2/form/TitleImplV2.java

+    @Nullable
+    private String title;
+
+    @ValueMapValue(injectionStrategy = InjectionStrategy.OPTIONAL, name = "format")


Please rename this, and add it in the ReservedProperties class, you can check other class for references

Implemented!

adobe-bot · 2024-07-16T08:54:02Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-07-16T08:54:02Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	92	96	96	75

adobe-bot · 2024-07-16T08:54:13Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-16T08:56:37Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-16T08:57:12Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-29T12:18:32Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	92	96	96	75

adobe-bot · 2024-07-29T12:18:32Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-07-29T12:18:43Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-29T12:19:06Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-07-29T12:20:27Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

rismehta · 2024-07-31T09:22:53Z

ui.tests/test-module/specs/title/titleV2.authoring.spec.js

+          cy.get(titlePolicy).click({force: true});
+          cy.get(bemDesignDialog).contains('Title').click();
+          cy.get('[name="./type"]').eq(0).click({ force: true }).then(() => {
+            cy.get('._coral-Menu-item').contains(defaultValue).click({ force: true });


Please don't make use of private coral selectors in the spec, the spec would break in release/650

rismehta · 2024-07-31T09:25:21Z

ui.tests/test-module/specs/title/titleV2.authoring.spec.js

+            cy.get('[placeholder="New policy"]').eq(1).type("Default policy");
+            cy.get('[title="Done"]').click();
+          }).then(() => {
+            cy.openSiteAuthoring(authoringPagePath);    


Can you improve the test case to make use of the policy configured in design dialog in the form editor via title's edit dialog ?

rismehta · 2024-07-31T09:25:43Z

ui.tests/test-module/specs/title/titleV2.authoring.spec.js

+        });
+
+        it('check edit dialog availability of Title', function () {
+            testTitleEditDialog(titleEditPathSelector, titleDrop, true);


Can also write a runtime test case for v2 ?

rismehta

check comments

adobe-bot · 2024-08-01T08:28:20Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	91	96	96	75

adobe-bot · 2024-08-01T08:28:20Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-08-01T08:28:30Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-08-01T08:30:32Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-08-01T08:30:50Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-08-05T03:23:48Z

Lighthouse scores (desktop)

	Performance	Accessibility	Best-Practices	SEO
Scores	100	96	96	75

adobe-bot · 2024-08-05T03:23:48Z

Lighthouse scores (mobile)

	Performance	Accessibility	Best-Practices	SEO
Scores	92	96	96	75

adobe-bot · 2024-08-05T03:23:59Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-08-05T03:24:51Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

adobe-bot · 2024-08-05T03:26:05Z

Accessibility Violations Found

Id	Impact
label-title-only	serious
target-size	serious

Co-authored-by: Pavitra Khatri <pavitrakhatri@Pavitras-MacBook-Pro.local>

github-actions bot force-pushed the title/fieldtype branch from d40f36f to 14748ed Compare June 27, 2024 03:37

pavi41 force-pushed the title/fieldtype branch from 14748ed to e8c1722 Compare June 27, 2024 04:23

github-actions bot force-pushed the title/fieldtype branch from e8c1722 to 4f21f01 Compare June 27, 2024 04:23

pavi41 force-pushed the title/fieldtype branch from 4f21f01 to f679b33 Compare June 27, 2024 04:39

github-actions bot force-pushed the title/fieldtype branch from f679b33 to a6b6486 Compare June 27, 2024 04:39

pavi41 force-pushed the title/fieldtype branch from a6b6486 to b5a4af3 Compare July 2, 2024 05:43

github-actions bot force-pushed the title/fieldtype branch from b5a4af3 to cad143c Compare July 2, 2024 05:43

pavi41 force-pushed the title/fieldtype branch from cad143c to b5a4af3 Compare July 2, 2024 08:29

github-actions bot force-pushed the title/fieldtype branch from b5a4af3 to c59cd3a Compare July 2, 2024 08:29

rismehta requested changes Jul 3, 2024

View reviewed changes

pavi41 force-pushed the title/fieldtype branch from c59cd3a to 09eae89 Compare July 16, 2024 07:21

github-actions bot force-pushed the title/fieldtype branch from 09eae89 to 1263e84 Compare July 16, 2024 07:21

pavi41 force-pushed the title/fieldtype branch from 1263e84 to c7f77a7 Compare July 16, 2024 08:30

github-actions bot force-pushed the title/fieldtype branch from c7f77a7 to 07003d2 Compare July 16, 2024 08:30

pavi41 force-pushed the title/fieldtype branch from debd801 to 13a8f0e Compare July 29, 2024 11:52

github-actions bot force-pushed the title/fieldtype branch from 13a8f0e to 32d5ec6 Compare July 29, 2024 11:52

rismehta reviewed Jul 31, 2024

View reviewed changes

rismehta requested changes Jul 31, 2024

View reviewed changes

pavi41 force-pushed the title/fieldtype branch from 32d5ec6 to d912437 Compare August 1, 2024 08:03

github-actions bot force-pushed the title/fieldtype branch from d912437 to 16ee208 Compare August 1, 2024 08:04

pavi41 force-pushed the title/fieldtype branch from 16ee208 to b5d4555 Compare August 5, 2024 02:59

V2 version of Title

c65f065

github-actions bot force-pushed the title/fieldtype branch from b5d4555 to c65f065 Compare August 5, 2024 02:59

rismehta approved these changes Aug 6, 2024

View reviewed changes

rismehta merged commit 22b0a56 into dev Aug 6, 2024

rismehta deleted the title/fieldtype branch August 6, 2024 10:23

dmaurya929 pushed a commit that referenced this pull request Aug 13, 2024

V2 version of Title (#1292)

4ddff6a

Co-authored-by: Pavitra Khatri <pavitrakhatri@Pavitras-MacBook-Pro.local>

V2 version of Title #1292

V2 version of Title #1292

Uh oh!

Conversation

pavi41 commented Jun 27, 2024

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov bot commented Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adobe-bot commented Jul 2, 2024

Lighthouse scores (mobile)

Uh oh!

adobe-bot commented Jul 2, 2024

Lighthouse scores (desktop)

Uh oh!

adobe-bot commented Jul 2, 2024

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 2, 2024

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 2, 2024

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 2, 2024

Lighthouse scores (desktop)

Uh oh!

adobe-bot commented Jul 2, 2024

Lighthouse scores (mobile)

Uh oh!

adobe-bot commented Jul 2, 2024

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 2, 2024

Accessibility Violations Found

Uh oh!

adobe-bot commented Jul 2, 2024

Accessibility Violations Found

Uh oh!

rismehta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdua Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 27, 2024 •

edited

Loading

vdua Jul 10, 2024 •

edited

Loading