Merge pull request #1150 from Patternslib/scrum-1106--review

Implement review comments

Author: thet

src/core/utils.js

@@ -447,7 +447,7 @@ function parseTime(time) {
  * parseLength - Parse a length from a string and return the parsed length in
  * pixels.
 
- * @param {String} length - A length string like `1px` or `25%`.
+ * @param {String} length - A length string like `1px` or `25%`. Lengths without a unit are treated as pixels.
  * @param {Number} reference_length - The reference length to use for percentage lengths.
  *
  * @returns {Number} - A integer which represents the parsed length in pixels.
@@ -486,7 +486,7 @@ function parseLength(length, reference_length = null) {
                 (amount * Math.max(window.innerWidth, window.innerHeight)) / 100
             );
         default:
-            return null;
+            return Math.round(amount);
     }
 }
 
@@ -756,6 +756,36 @@ const threshold_list = (num_steps = 0) => {
     return thresholds.sort();
 };
 
+/**
+ * is_option_truthy - Check if an Pattern option is set.
+ *
+ * An option is set if it is not one of:
+ * - undefined
+ * - null
+ * - "none"
+ * - ""
+ *
+ * @param {String} option - The option to check.
+ *
+ * @returns {Boolean} - Returns true if the option is set, false otherwise.
+ *
+ * @example
+ *
+ * is_option_truthy() // false
+ * is_option_truthy(undefined) // false
+ * is_option_truthy(null) // false
+ * is_option_truthy("") // false
+ * is_option_truthy("none") // false
+ * is_option_truthy("false") // false
+ * is_option_truthy("foo") // true
+ * is_option_truthy(true) // true
+ * is_option_truthy(0) // true
+ *
+ */
+const is_option_truthy = (option) => {
+    return ![undefined, null, "none", false, "false", ""].includes(option);
+};
+
 var utils = {
     jqueryPlugin: jqueryPlugin,
     escapeRegExp: escapeRegExp,
@@ -789,6 +819,7 @@ var utils = {
     is_iso_date: is_iso_date,
     date_diff: date_diff,
     threshold_list: threshold_list,
+    is_option_truthy: is_option_truthy,
     getCSSValue: dom.get_css_value, // BBB: moved to dom. TODO: Remove in upcoming version.
 };
 

src/core/utils.test.js

@@ -480,6 +480,24 @@ describe("parseLength", function () {
         expect(p).toThrow();
     });
 
+    it("handles unitless lengths", function () {
+        // As strings
+        expect(utils.parseLength("0")).toBe(0);
+        expect(utils.parseLength("1")).toBe(1);
+        expect(utils.parseLength("10")).toBe(10);
+        expect(utils.parseLength("100")).toBe(100);
+        expect(utils.parseLength("1000.1")).toBe(1000);
+        expect(utils.parseLength("1000.9")).toBe(1001);
+
+        // As numbers
+        expect(utils.parseLength(0)).toBe(0);
+        expect(utils.parseLength(1)).toBe(1);
+        expect(utils.parseLength(10)).toBe(10);
+        expect(utils.parseLength(100)).toBe(100);
+        expect(utils.parseLength(1000.1)).toBe(1000);
+        expect(utils.parseLength(1000.9)).toBe(1001);
+    });
+
     it("handles pixel lengths", function () {
         expect(utils.parseLength("10px")).toBe(10);
         expect(utils.parseLength("100px")).toBe(100);
@@ -881,3 +899,24 @@ describe("threshold_list ...", function () {
         ]);
     });
 });
+
+describe("is_option_truthy ...", function () {
+    it("checks if an option is set", () => {
+        // These options are considered truthy
+        expect(utils.is_option_truthy("a")).toBe(true);
+        expect(utils.is_option_truthy(true)).toBe(true);
+        expect(utils.is_option_truthy("true")).toBe(true);
+        expect(utils.is_option_truthy(1)).toBe(true);
+        // Also "0" is considered truthy because it can be a valid option for
+        // length, time and so forth.
+        expect(utils.is_option_truthy(0)).toBe(true);
+
+        // These options are considered falsy
+        expect(utils.is_option_truthy(undefined)).toBe(false);
+        expect(utils.is_option_truthy(null)).toBe(false);
+        expect(utils.is_option_truthy(false)).toBe(false);
+        expect(utils.is_option_truthy("false")).toBe(false);
+        expect(utils.is_option_truthy("none")).toBe(false);
+        expect(utils.is_option_truthy("")).toBe(false);
+    });
+});

src/pat/navigation/documentation.md

@@ -26,15 +26,15 @@ This would invoke a `click` event on the current navigation item and that can be
 The navigation pattern can be configured through a `data-pat-navigation` attribute.
 The available options are:
 
-| Field                    | Default              | Options                   | Description                                                                                                                                                              |
-| ------------------------ | -------------------- | ------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------ |
-| item-wrapper             | `li`                   | CSS selector              | The DOM element which wraps each menu item. This is used to set the "current" and "in-path" classes also on the wrapper element. If empty, no wrapper element is used.   |
-| in-path-class            | `navigation-in-path`   | CSS class name            | Class name, which is set on item-wrapper elements if nested menu items are within the current path.                                                                      |
-| current-class            | `current`              | CSS class name            | Class name, which is set on item-wrapper or items if they are the currently selected menu option or - for hash links - when it's corresponding content item is in view.  |
-| in-view-class            | `in-view`              |                           | CSS class for a navigation item when it's corresponding content item is in view.                                                                                         |
-| current-content-class    | `current`              |                           | CSS class for a content item when it is the current one.                                                                                                                 |
-| scroll-item-side       | `top`                  | top, bottom, middle, auto | Side of element that scrolls. This is used to calculate the current item. The defined side of the element will be used to calculate the distance baseline. If this is set to "auto" then one of the "top" or "bottom" positions are used, depending on which one is nearer to the distance baseline. |
-| scroll-item-distance   | `50%`                  |                           | Distance from side of scroll box. any amount in px or %. This is used to calculate the current item. The nearest element to the distance-baseline measured from the top of the container will get the current class. |
-| scroll-item-visibility |                      | none, most-visible        | Visibility of the item/element in scroll box. This is used to calculate the current item. If "most-visible" is set, the element which is most visible in the scroll container gets the current class. If more elements have the same visibility ratio, the other conditions are used to calculate the current one. |
-| scroll-trigger-selector | `a[href^='#'].scroll-marker`     | CSS selector, none             | Selects the element within the pat-navigation container that should get a class current while scrolling, when applicable. |
+| Field                   | Default                      | Options                                  | Description             |
+| ----------------------- | ---------------------------- | ---------------------------------------- | ----------------------- |
+| item-wrapper            | `li`                         | CSS selector                             | The DOM element which wraps each menu item. This is used to set the "current" and "in-path" classes also on the wrapper element. If empty, no wrapper element is used. |
+| in-path-class           | `navigation-in-path`         | CSS class name                           | Class name, which is set on item-wrapper elements if nested menu items are within the current path. |
+| current-class           | `current`                    | CSS class name                           | Class name, which is set on item-wrapper or items if they are the currently selected menu option or - for hash links - when it's corresponding content item is in view. |
+| current-content-class   | `current`                    | CSS class name                           | CSS class for a content item when it is the current one. |
+| in-view-class           | `in-view`                    | CSS class name                           | CSS class for a navigation item when it's corresponding content item is in view. |
+| scrill-item-side        | `top`                        | `top`, `bottom`, `middle`, `auto`        | Side of element that scrolls. This is used to calculate the current item. The defined side of the element will be used to calculate the distance baseline. If this is set to "auto" then one of the "top" or "bottom" positions are used, depending on which one is nearer to the distance baseline. |
+| scrill-item-distance    | `50%`                        | CSS length (px, %, vw, vh, vmin or vmax) | Distance from side of scroll box. any amount in px, %, vw, vh, vmin or vmax. This is used to calculate the current item. The nearest element to the distance-baseline measured from the top of the container will get the current class. |
+| scrill-item-visibility  |                              | `none`, `most-visible`                   | Visibility of element in scroll box. This is used to calculate the current item. If "most-visible" is set, the element which is most visible in the scroll container gets the current class. If more elements have the same visibility ratio, the other conditions are used to calculate the current one. |
+| scroll-trigger-selector | `a[href^='#'].scroll-marker` | CSS selector, `none`                     | Selects the element within the pat-navigation container that should get a class current while scrolling, when applicable. |
 

src/pat/navigation/navigation.js

@@ -17,12 +17,10 @@ parser.addArgument("in-view-class", "in-view");
 parser.addArgument("current-class", "current");
 parser.addArgument("current-content-class", "navigation-current");
 
-// Side of element that scrolls. top/bottom/middle/auto (default 'top')
-parser.addArgument("scroll-marker-side", "top", ["top", "bottom", "middle", "auto"]);
-// Distance from side of scroll box. any amount in px or % (default '50%')
-parser.addArgument("scroll-marker-distance", "50%");
-// Visibility of element in scroll box. most-visible or null (default null)
-parser.addArgument("scroll-marker-visibility", null, [null, "most-visible"]);
+parser.addArgument("scroll-item-side", "top", ["top", "bottom", "middle", "auto"]);
+parser.addArgument("scroll-item-distance", "50%");
+parser.addArgument("scroll-item-visibility", null, [null, "none", "most-visible"]);
+parser.addArgument("scroll-trigger-selector", "a[href^='#'].scroll-marker");
 
 class Pattern extends BasePattern {
     static name = "navigation";
@@ -37,25 +35,28 @@ class Pattern extends BasePattern {
         this.init_listeners();
         this.init_markings();
 
-        this.scroll_marker = new ScrollMarker(this.el, {
-            "current-class": this.options["current-class"],
-            "current-content-class": this.options["current-content-class"],
-            "in-view-class": this.options["in-view-class"],
-            "side": this.options["scroll-marker-side"],
-            "distance": this.options["scroll-marker-distance"],
-            "visibility": this.options["scroll-marker-visibility"],
-        });
+        if (utils.is_option_truthy(this.options["scroll-trigger-selector"])) {
+            this.scroll_marker = new ScrollMarker(this.el, {
+                "current-class": this.options["current-class"],
+                "current-content-class": this.options["current-content-class"],
+                "in-view-class": this.options["in-view-class"],
+                "side": this.options["scroll-item-side"],
+                "distance": this.options["scroll-item-distance"],
+                "visibility": this.options["scroll-item-visibility"],
+                "selector": this.options["scroll-trigger-selector"],
+            });
 
-        this.debounced_scroll_marker_enabler = utils.debounce(() => {
-            log.debug("Enable scroll-marker.");
-            this.scroll_marker.set_current_disabled = false;
-            events.remove_event_listener(
-                this.scroll_marker.scroll_container === window
-                    ? document
-                    : this.scroll_marker.scroll_container,
-                "pat-navigation__scroll_marker_enable"
-            );
-        }, 200);
+            this.debounced_scroll_marker_enabler = utils.debounce(() => {
+                log.debug("Enable scroll-marker.");
+                this.scroll_marker.set_current_disabled = false;
+                events.remove_event_listener(
+                    this.scroll_marker.scroll_container === window
+                        ? document
+                        : this.scroll_marker.scroll_container,
+                    "pat-navigation__scroll_marker_enable"
+                );
+            }, 200);
+        }
     }
 
     /**
@@ -75,6 +76,12 @@ class Pattern extends BasePattern {
                     // Mark the current item
                     this.mark_current(ev.target);
 
+                    if (
+                        !utils.is_option_truthy(this.options["scroll-trigger-selector"])
+                    ) {
+                        // Don't do the scroll-marker stuff below, if it is disabled by the settings.
+                        return;
+                    }
                     // Disable scroll marker to set the current class after
                     // clicking in the menu and scrolling to the target.
                     log.debug("Disable scroll-marker.");

src/pat/scroll-marker/documentation.md

@@ -48,11 +48,13 @@ Here is a complete example:
 
 ### Options reference
 
-| Property       | Default Value | Values | Type              | Description                   |
-| -------------- | ------------- | ------ | ----------------- | ----------------------------- |
-| in-view-class  | in-view       |        | String            | CSS class for a navigation item when it's corresponding content item is in view. |
-| current-class  | current       |        | String            | CSS class for a navigation item when it's corresponding content item is the current one. |
-| current-content-class | current |       | String            | CSS class for a content item when it is the current one. |
-| side           | top           | top, bottom, middle, auto  | String | Side of element that scrolls. This is used to calculate the current item. The defined side of the element will be used to calculate the distance baseline. If this is set to "auto" then one of the "top" or "bottom" positions are used, depending on which one is nearer to the distance baseline. |
-| distance       | 50%           |        | String            | Distance from side of scroll box. any amount in px, %, vw, vh, vmin or vmax. This is used to calculate the current item. The nearest element to the distance-baseline measured from the top of the container will get the current class. |
-| visibility     |               | null, most-visible | String | Visibility of element in scroll box. This is used to calculate the current item. If "most-visible" is set, the element which is most visible in the scroll container gets the current class. If more elements have the same visibility ratio, the other conditions are used to calculate the current one. |
+| Property              | Default        | Options                                  | Description                   |
+| --------------------- | -------------- | ---------------------------------------- | ----------------------------- |
+| current-class         | `current`      | CSS class name                           | CSS class for a navigation item when it's corresponding content item is the current one. |
+| current-content-class | `current`      | CSS class name                           | CSS class for a content item when it is the current one. |
+| in-view-class         | `in-view`      | CSS class name                           | CSS class for a navigation item when it's corresponding content item is in view. |
+| side                  | `top`          | `top`, `bottom`, `middle`, `auto`        | Side of element that scrolls. This is used to calculate the current item. The defined side of the element will be used to calculate the distance baseline. If this is set to "auto" then one of the "top" or "bottom" positions are used, depending on which one is nearer to the distance baseline. |
+| distance              | `50%`          | CSS length (px, %, vw, vh, vmin or vmax) | Distance from side of scroll box. any amount in px, %, vw, vh, vmin or vmax. This is used to calculate the current item. The nearest element to the distance-baseline measured from the top of the container will get the current class. |
+| visibility            |                | `none`, `most-visible`                   | Visibility of element in scroll box. This is used to calculate the current item. If "most-visible" is set, the element which is most visible in the scroll container gets the current class. If more elements have the same visibility ratio, the other conditions are used to calculate the current one. |
+| selector              | `a[href^='#']` | CSS selector, `none`                     | Selects the element within the pat-navigation container that should get a class current while scrolling, when applicable. |
+

src/pat/scroll-marker/scroll-marker.js

@@ -13,12 +13,10 @@ parser.addArgument("in-view-class", "in-view");
 parser.addArgument("current-class", "current");
 parser.addArgument("current-content-class", "scroll-marker-current");
 
-// Side of element that scrolls. top/bottom/middle/auto (default 'top')
 parser.addArgument("side", "top", ["top", "bottom", "middle", "auto"]);
-// Distance from side of scroll box. any amount in px, %, vw, vh, vmin or vmax (default '50%')
 parser.addArgument("distance", "50%");
-// Visibility of element in scroll box. most-visible or null (default null)
-parser.addArgument("visibility", null, [null, "most-visible"]);
+parser.addArgument("visibility", null, [null, "none", "most-visible"]);
+parser.addArgument("selector", "a[href^='#']");
 
 class Pattern extends BasePattern {
     static name = "scroll-marker";
@@ -34,7 +32,7 @@ class Pattern extends BasePattern {
     init() {
         // Get all elements that are referenced by links in the current page.
         this.observables = new Map(
-            [...dom.querySelectorAllAndMe(this.el, "a[href^='#']")]
+            [...dom.querySelectorAllAndMe(this.el, this.options.selector)]
                 .map(
                     // Create the structure:
                     // id: {link, target}

src/pat/scroll-marker/scroll-marker.test.js

@@ -232,6 +232,30 @@ describe("pat-scroll-marker", () => {
             expect(id2.classList.contains("scroll-marker-current")).toBe(true);
             expect(id3.classList.contains("scroll-marker-current")).toBe(false);
         });
+
+        it("1.5: The selector option can exclude some items", async () => {
+            // With the selector option you can include/exclude some items from
+            // being included in the scroll-marker functionality.
+
+            const { nav_id1, nav_id2, nav_id3, id1, id2, id3 } =
+                await create_scroll_marker({
+                    options: {
+                        selector: "a[href^='#']:not([href='#id3'])",
+                    },
+                });
+
+            expect(nav_id1.classList.contains("in-view")).toBe(false);
+            expect(nav_id2.classList.contains("in-view")).toBe(true);
+            expect(nav_id3.classList.contains("in-view")).toBe(false);
+
+            expect(nav_id1.classList.contains("current")).toBe(false);
+            expect(nav_id2.classList.contains("current")).toBe(true);
+            expect(nav_id3.classList.contains("current")).toBe(false);
+
+            expect(id1.classList.contains("scroll-marker-current")).toBe(false);
+            expect(id2.classList.contains("scroll-marker-current")).toBe(true);
+            expect(id3.classList.contains("scroll-marker-current")).toBe(false);
+        });
     });
 
     describe("2: Test on element as scroll container", () => {