[GitHub] [camel-website] zregvart commented on a diff in pull request #995: Updated rules.js

["zregvart (via GitHub)" <gi...@apache.org>]

zregvart commented on code in PR #995:
URL: https://github.com/apache/camel-website/pull/995#discussion_r1173472140


##########
rules.js:
##########
@@ -36,15 +40,14 @@ class RelativeLinks extends Rule {
   setup() {
     this.on("dom:ready", event => {
       const anchors = event.document.getElementsByTagName('a');
-      if (anchors === null) {
-        return;
-      }
-      anchors.forEach(a => {
-        const href = a.getAttribute("href");
-        if (href && href.value.startsWith("https://camel.apache.org")) {
-          this.report(a, `For links within camel.apache.org use relative links, found: ${href.value}`);
+      if (anchors !== null) {

Review Comment:
   I prefer the non-nested variant, i.e.
   
   ```javascript
   if (anchors === null) {
     return;
   }
   ```
   
   This [blog post](https://testing.googleblog.com/2017/06/code-health-reduce-nesting-reduce.html) articulates the reasons why.



##########
rules.js:
##########
@@ -6,26 +6,30 @@ class HtmlTitle extends Rule {
       description: "Title of the page must be defined"
     };
   }
+  validateTitle(title) {

Review Comment:
   If we're going with this approach, I'd prefer putting all the logic in the `validateTitle` function.
   
   ```javascript
   validateTitle(event) {
   //...
   }
   ```
   
   And below we would have:
   
   ```javascript
   this.on("dom:ready", validateTitle)
   ```



##########
rules.js:
##########
@@ -36,15 +40,14 @@ class RelativeLinks extends Rule {
   setup() {
     this.on("dom:ready", event => {
       const anchors = event.document.getElementsByTagName('a');
-      if (anchors === null) {
-        return;
-      }
-      anchors.forEach(a => {
-        const href = a.getAttribute("href");
-        if (href && href.value.startsWith("https://camel.apache.org")) {
-          this.report(a, `For links within camel.apache.org use relative links, found: ${href.value}`);
+      if (anchors !== null) {
+        for (let i = 0; i < anchors.length; i++) {
+          const href = anchors[i].getAttribute("href");
+          if (href && href.startsWith("https://camel.apache.org")) {

Review Comment:
   Missing `href.value` here
   
   ```suggestion
             if (href && href.value.startsWith("https://camel.apache.org")) {
   ```



##########
rules.js:
##########
@@ -56,21 +59,12 @@ class StructuredData extends Rule {
     };
   }
 
-  setup() {
-    let start = -1;
-
-    this.on('tag:open', event => {
-      if (event.target.nodeName === 'script') {
-        start = event.target.location.offset;
-      }
-    });
-
-    this.on('tag:close', async event => {
-      const tag = event.previous;
-      if (event.target.nodeName === 'script' && tag.hasAttribute('type') && tag.getAttribute('type').value === 'application/ld+json') {
-        const startIdx = data.indexOf('>', start) + 1;
-        const endIdx = event.target.location.offset - 1; // omit the opening tag angled bracket
-        const content = data.substring(startIdx, endIdx);
+  
+setup() {
+    this.on('tag:close', event => {
+      const tag = event.target;

Review Comment:
   For `tag:close` the event doesn't have a `target` property and that's why we need to remember the offset location of when the tag was opened. In the `tag:open` we also don't have access to attributes. This is why the logic here how it is.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org