CIF-2188 - Expose HTML id attribute in component edit dialogs #639

LSantha · 2021-07-26T14:00:11Z

adding field for HTML id attribute in edit dialog of components
reusing the HTML id generation of core WCM components
updated unit tests

Related Issue

CIF-2188

How Has This Been Tested?

JUnit, manually.

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.

* adding field for HTML id attribute in edit dialog of components * reusing the HTML id generation of core WCM components * updated unit tests

* fixing IT failure

codecov · 2021-07-26T14:48:45Z

Codecov Report

Merging #639 (9788641) into master (218d544) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #639      +/-   ##
============================================
+ Coverage     88.28%   88.29%   +0.01%     
- Complexity     1581     1584       +3     
============================================
  Files           279      279              
  Lines          6933     6940       +7     
  Branches       1036     1037       +1     
============================================
+ Hits           6121     6128       +7     
  Misses          611      611              
  Partials        201      201

Flag	Coverage Δ
integration	`59.89% <61.11%> (-0.11%)`	⬇️
jest	`85.61% <ø> (ø)`
karma	`89.05% <ø> (ø)`
unittests	`88.80% <94.44%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...e/components/internal/datalayer/AssetDataImpl.java	`100.00% <100.00%> (ø)`
...omponents/internal/datalayer/CategoryDataImpl.java	`84.61% <100.00%> (ø)`
...ponents/internal/datalayer/DataLayerComponent.java	`75.60% <100.00%> (+1.25%)`	⬆️
...mponents/internal/datalayer/DataLayerListItem.java	`100.00% <100.00%> (ø)`
...internal/models/v1/common/ProductListItemImpl.java	`97.77% <100.00%> (ø)`
...1/contentfragment/CommerceContentFragmentImpl.java	`83.49% <100.00%> (+0.16%)`	⬆️
...onents/internal/models/v1/product/ProductImpl.java	`89.11% <100.00%> (+0.38%)`	⬆️
.../converters/ProductToProductListItemConverter.java	`92.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 218d544...9788641. Read the comment docs.

mhaack · 2021-07-28T08:20:15Z

...c/main/java/com/adobe/cq/commerce/core/components/internal/datalayer/DataLayerComponent.java

 import com.fasterxml.jackson.annotation.JsonIgnore;

 public abstract class DataLayerComponent {
-    public static final String ID_SEPARATOR = "-";
+    public static final String ID_SEPARATOR = ComponentUtils.ID_SEPARATOR;


Why redefining the const instead of using ComponentUtils.ID_SEPARATOR directly via static import?

The ID_SEPARATOR is used at more other places in the codebe in sub-classes of DataLayerComponnet. If you define it here you don't need any static import in many other files. In fact the constant was defined earlier, I've just aligned the definition with ComponentUtils.

I know the constant was defined earlier and it was wrong. So instead of having the same constant twice I would prefer to remove it and only use ComponentUtils.ID_SEPARATOR. The static import should not hurt.

+1 - this is more explicit.

I've inlined the ID_SEPARATOR constant.

buuhuu · 2021-07-28T13:23:33Z

...c/main/java/com/adobe/cq/commerce/core/components/internal/datalayer/DataLayerComponent.java

 import com.fasterxml.jackson.annotation.JsonIgnore;

 public abstract class DataLayerComponent {
-    public static final String ID_SEPARATOR = "-";
+    public static final String ID_SEPARATOR = ComponentUtils.ID_SEPARATOR;


+1 - this is more explicit.

buuhuu · 2021-07-28T13:26:25Z

...c/main/java/com/adobe/cq/commerce/core/components/internal/datalayer/DataLayerComponent.java

-        String prefix = StringUtils.substringAfterLast(resourceType, "/");
-        String path = resource.getPath();
-        return StringUtils.join(prefix, ID_SEPARATOR, StringUtils.substring(DigestUtils.sha256Hex(path), 0, 10));
+        if (wcmComponent == null) {


how can wcmComponent be null?

We only have 2 models using an optional default injection strategy (button v1 and v2). For all others the wcmComponent is required.

DataLayerComponent is also extended by classes which are not Sling models, we handle that case here.

Can you add this detail to the javadoc of the class while you are on it?

What detail?

That this abstract class may be extended by classes that are annotated as sling models and others that are not.

* cleaned up ID_SEPARATOR

LSantha added 2 commits July 26, 2021 16:58

CIF-2188 - Expose HTML id attribute in component edit dialogs

5144ae1

* adding field for HTML id attribute in edit dialog of components * reusing the HTML id generation of core WCM components * updated unit tests

CIF-2188 - Expose HTML id attribute in component edit dialogs

2e93482

* fixing IT failure

LSantha added the enhancement New feature or request label Jul 26, 2021

LSantha requested review from mhaack, dplaton, buuhuu, laurentiumagureanu and herzog31 July 26, 2021 14:54

herzog31 approved these changes Jul 28, 2021

View reviewed changes

mhaack reviewed Jul 28, 2021

View reviewed changes

dplaton approved these changes Jul 28, 2021

View reviewed changes

Merge branch 'master' into CIF-2188

d78d820

buuhuu reviewed Jul 28, 2021

View reviewed changes

CIF-2188 - Expose HTML id attribute in component edit dialogs

2cad8b9

* cleaned up ID_SEPARATOR

mhaack approved these changes Jul 28, 2021

View reviewed changes

Merge branch 'master' into CIF-2188

9788641

laurentiumagureanu approved these changes Jul 30, 2021

View reviewed changes

laurentiumagureanu merged commit a855391 into master Jul 30, 2021

laurentiumagureanu deleted the CIF-2188 branch July 30, 2021 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CIF-2188 - Expose HTML id attribute in component edit dialogs #639

CIF-2188 - Expose HTML id attribute in component edit dialogs #639

LSantha commented Jul 26, 2021

codecov bot commented Jul 26, 2021 •

edited

Loading

mhaack Jul 28, 2021

LSantha Jul 28, 2021

mhaack Jul 28, 2021

buuhuu Jul 28, 2021

LSantha Jul 28, 2021

buuhuu Jul 28, 2021

buuhuu Jul 28, 2021

LSantha Jul 28, 2021

buuhuu Jul 28, 2021

LSantha Jul 28, 2021

buuhuu Jul 28, 2021

CIF-2188 - Expose HTML id attribute in component edit dialogs #639

CIF-2188 - Expose HTML id attribute in component edit dialogs #639

Conversation

LSantha commented Jul 26, 2021

Related Issue

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Jul 26, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2021 •

edited

Loading