Table of Contents Component #2030

vsarya · 2022-02-22T09:44:59Z

Q	A
Fixed Issues?	https://github.com/adobe/aem-core-wcm-components/wiki/Table-of-Content-Component
Patch: Bug Fix?	No
Minor: New Feature?	Yes
Major: Breaking Change?	No
Tests Added + Pass?	UTs and UI Selenium tests added
Documentation Provided	Yes
Any Dependency Changes?	Yes (JSOUP)
License	Apache License, Version 2.0

lgtm-com · 2022-02-22T10:13:34Z

This pull request introduces 1 alert when merging caa947a into 1b0503c - view on LGTM.com

new alerts:

1 for Useless comparison test

lgtm-com · 2022-02-23T20:42:09Z

This pull request introduces 1 alert when merging d4750e5 into 7434de3 - view on LGTM.com

new alerts:

1 for Useless comparison test

codecov · 2022-02-24T22:31:23Z

Codecov Report

Merging #2030 (35a0812) into main (d6ba9ab) will increase coverage by 0.16%.
The diff coverage is 92.00%.

❗ Current head 35a0812 differs from pull request most recent head dab526b. Consider uploading reports for the commit dab526b to get more accurate results

@@             Coverage Diff              @@
##               main    #2030      +/-   ##
============================================
+ Coverage     86.60%   86.77%   +0.16%     
- Complexity     2313     2375      +62     
============================================
  Files           210      214       +4     
  Lines          6161     6361     +200     
  Branches        931      964      +33     
============================================
+ Hits           5336     5520     +184     
- Misses          329      336       +7     
- Partials        496      505       +9

Impacted Files	Coverage Δ
...cq/wcm/core/components/models/TableOfContents.java	`88.88% <88.88%> (ø)`
...nents/internal/servlets/TableOfContentsFilter.java	`90.90% <90.90%> (ø)`
...onents/internal/models/v1/TableOfContentsImpl.java	`100.00% <100.00%> (ø)`
...ponents/internal/servlets/CharResponseWrapper.java	`100.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 d6ba9ab...dab526b. Read the comment docs.

lgtm-com · 2022-02-24T22:52:32Z

This pull request introduces 1 alert when merging 916d303 into 0767db9 - view on LGTM.com

new alerts:

1 for Useless comparison test

lgtm-com · 2022-02-25T11:37:19Z

This pull request introduces 1 alert when merging 530f8a1 into 0767db9 - view on LGTM.com

new alerts:

1 for Useless comparison test

lgtm-com · 2022-02-28T12:27:13Z

This pull request introduces 1 alert when merging b59c166 into f4c2da2 - view on LGTM.com

new alerts:

1 for Useless comparison test

jckautzmann · 2022-03-01T16:46:41Z

...e/src/main/java/com/adobe/cq/wcm/core/components/internal/models/v1/TableOfContentsImpl.java

+        restrictStopLevel = currentStyle.get(PN_RESTRICT_STOP_LEVEL, String.class);
+        includeClasses = currentStyle.get(PN_INCLUDE_CLASSES, String[].class);
+        ignoreClasses = currentStyle.get(PN_IGNORE_CLASSES, String[].class);
+        slingHttpServletRequest.setAttribute("contains-table-of-contents", true);


I'd prefix with cmp to avoid clashes with custom names: cmp:contains-table-of-contents

...re/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/CharResponseWrapper.java

.../src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/TableOfContentsFilter.java

jckautzmann · 2022-03-01T17:29:53Z

It would be nice to add a few Selenium tests.

vladbailescu

Please also add a README file in the version root folder, similar to other components!

vladbailescu · 2022-03-02T07:56:43Z

.../src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/TableOfContentsFilter.java

+        CharResponseWrapper responseWrapper = new CharResponseWrapper((HttpServletResponseWrapper) response);
+        chain.doFilter(request, responseWrapper);
+        String originalContent = responseWrapper.toString();
+        Boolean containsTableOfContents = (Boolean) request.getAttribute("contains-table-of-contents");


Please isolate all "magic strings" into constants. Also, for Core Components we generally use BEM-style classes (see https://github.com/adobe/aem-core-wcm-components/wiki/css-coding-conventions) so this class would be best refactored to cmp-toc__placeholder

While I understand to use the BEM style for all the CSS classnames used in this PR, this one is not a CSS class but a request attribute. Do we use the same style for request attributes as well or just prepend it with cmp: as pointed by @jckautzmann here?

vladbailescu · 2022-03-02T07:59:36Z

bundles/core/src/main/java/com/adobe/cq/wcm/core/components/models/TableOfContents.java

+     * @since com.adobe.cq.wcm.core.components.models.tableofcontents 1.0
+     */
+    default String getListType() {
+        return "unordered";


Please define all possible values as constants in this class (or an enumeration)!

...rc/content/jcr_root/apps/core/wcm/components/tableofcontents/v1/tableofcontents/.content.xml

vladbailescu · 2022-03-02T08:05:15Z

...re/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/CharResponseWrapper.java

+import java.io.CharArrayWriter;
+import java.io.PrintWriter;
+
+public class CharResponseWrapper extends HttpServletResponseWrapper {


Will this work across multiple encodings?

This wrapper just overrides the writer of the original response. Rest everything is handled by the original response object.
CharResponseWrapper extends HttpServletResponseWrapper which further extends ServletResponseWrapper which has the method setCharacterEncoding.

vladbailescu · 2022-03-02T08:27:04Z

bundles/core/pom.xml

                            com.adobe.cq.commerce.*;resolution:=optional,
                            com.adobe.aem.wcm.seo.*;resolution:=optional,
                            javax.annotation;version=0.0.0,
+                            javax.annotation.meta;resolution:=optional,


Is this used?

No, only javax.annotation.* is used in this repo and only javax.annotation.PostConstruct is used in this PR.

lgtm-com · 2022-03-02T08:37:32Z

This pull request introduces 1 alert when merging 5fee6ff into e114d7c - view on LGTM.com

new alerts:

1 for Useless comparison test

jckautzmann · 2022-03-02T13:02:25Z

Please add a README for this component.

jckautzmann · 2022-03-02T13:03:34Z

Please add a page to the component library with various different configurations of the TOC.

...jcr_root/apps/core/wcm/components/tableofcontents/v1/tableofcontents/_cq_dialog/.content.xml

jckautzmann · 2022-03-02T13:11:03Z

...nt/jcr_root/apps/core/wcm/components/tableofcontents/v1/tableofcontents/tableofcontents.html

+  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/-->
+<div data-sly-use.toc="com.adobe.cq.wcm.core.components.models.TableOfContents"
+     data-sly-use.component="com.adobe.cq.wcm.core.components.models.Component"
+     class = "table-of-contents-placeholder"


See Vlad's remark above: the class name should be cmp-toc__placeholder

jckautzmann · 2022-03-02T13:15:48Z

...nt/jcr_root/apps/core/wcm/components/tableofcontents/v1/tableofcontents/tableofcontents.html

+     data-sly-use.component="com.adobe.cq.wcm.core.components.models.Component"
+     class = "table-of-contents-placeholder"
+     id = "${component.id}"
+     data-list-type = "${toc.listType}"


I'd prefix the data attribute with cmp-toc-:

data-cmp-toc-list-type data-cmp-toc-start-level data-cmp-toc-include-classes data-cmp-toc-ignore-classes

Duplicagte IDs, small letters in JSON model

…ES-3971

vsarya · 2022-03-24T18:51:30Z

Don’t allow authors to configure invalid options in the dialog by validating the options and preventing a start level that is bigger than the stop level.

@gabrielwalt All improvements except the one mentioned above are done. I'll be back working on this item after my next week's PTO.

cc - @jckautzmann , @vladbailescu

jckautzmann · 2022-04-05T10:41:38Z

@vsarya I noticed a shift in the customID index. If you have 3 titles with the same text, e.g. mytitle the generated IDs are currently : mytitle, mytitle-2 and mytitle-3. IMHO they should be: mytitle, mytitle-1 and mytitle-2.

vsarya · 2022-04-06T08:45:24Z

@vsarya I noticed a shift in the customID index. If you have 3 titles with the same text, e.g. mytitle the generated IDs are currently : mytitle, mytitle-2 and mytitle-3. IMHO they should be: mytitle, mytitle-1 and mytitle-2.

@jckautzmann Just pushed the required changes.
commit - acfbf89

jckautzmann · 2022-04-11T13:29:47Z

@vsarya Please fix the build error due to the baseline error:
https://github.com/adobe/aem-core-wcm-components/runs/5971672041?check_suite_focus=true

jckautzmann · 2022-04-11T13:51:47Z

@vsarya What is the status with the broken docs links (Github + Adobe)?

jckautzmann · 2022-04-11T13:56:01Z

I'd change the dialog validation message to: "Stop level is smaller than start level" as the tooltip applies to the stop level field.

vsarya · 2022-04-12T07:14:46Z

@jckautzmann The sonar check on the PR is failing due to duplicate lines of code in content/src/content/jcr_root/apps/core/wcm/components/search/v2/search/clientlibs/site/js/search.js

# Conflicts: # examples/ui.content/src/content/jcr_root/content/core-components-examples/library/core-structure/.content.xml

vsarya · 2022-04-13T07:33:34Z

@vsarya What is the status with the broken docs links (Github + Adobe)?

@jckautzmann I had already created a CQDOC issue for the same as discussed in our weekly meetings - https://jira.corp.adobe.com/browse/CQDOC-18998

sonarqubecloud · 2022-04-18T10:47:28Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

No Coverage information
14.6% Duplication

jckautzmann · 2022-04-18T11:14:57Z

Thx @vsarya for implementing this TOC component!

vsarya · 2022-04-19T09:00:11Z

Thx @vsarya for implementing this TOC component!

I enjoyed a lot working on it and learned a lot about core components and request filters during its development.

Thanks a lot @jckautzmann and @vladbailescu for guiding me throughout the whole development phase and while doing the POC.

Thank you Varun and @gabrielwalt for giving me the opportunity to work on such an interesting component.
Looking forward to develop more such core components in the future.

grubyak · 2022-06-28T12:30:24Z

.../src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/TableOfContentsFilter.java

+        chain.doFilter(request, responseWrapper);
+        String originalContent = responseWrapper.toString();
+        Boolean containsTableOfContents = (Boolean) request.getAttribute(TableOfContentsImpl.TOC_REQUEST_ATTR_FLAG);
+        if (responseWrapper.getContentType().contains("text/html") &&


@jckautzmann we observed NPE in this line during our on-call shift

@grubyak thx for your feedback. Could you please create a ticket describing the issue and how it can be reproduced? Thx!

This reverts commit ff2b76d, 503834b & 6f81f92.

vsarya mentioned this pull request Feb 22, 2022

Table of Contents Component #1972

Closed

vsarya changed the title ~~Main sites 3971~~ Table of Contents Component Feb 23, 2022

vsarya force-pushed the main--SITES-3971 branch from 6308e61 to df952c5 Compare February 25, 2022 10:55

vsarya force-pushed the main--SITES-3971 branch 3 times, most recently from b5cfed0 to b59c166 Compare February 28, 2022 11:57