Repository: obsidian-tasks-group/obsidian-tasks
Pull request #2442 (2023-11-22 13:43:21)

refactor: add data attributes in `TaskFieldRenderer` instead of returning them
<!--- Provide a general summary of your changes in the Title above -->

# Description

- `TaskFieldRenderer` now adds the data attributes to an `HTMLelement` instead of returning them (At the end of the day, this is what we need the data attributes for)
- Reworked tests to show this behaviour
- `TaskLineRenderer` calls to get the data attributes are shorter, `taskToHtml` more readable and comprehensive
- `AttributeDictionary` now used only in `TaskFieldRenderer.test.ts`

## Motivation and Context

- Simpler code
- Prepare to for further refactoring to make rendering and layout code more flexible towards new features
- Get rid of `DataAttributes` in `src` (Possible in `test` as well, but need to update tests, probably good for another PR)

## How has this been tested?

Modified unit tests.

## Types of changes

- [x] **Refactor** (prefix: `refactor` - non-breaking change which only improves the design or structure of existing code, and making no changes to its external behaviour)
- [x] **Tests** (prefix: `test` - additions and improvements to unit tests and the smoke tests)

## Checklist

- [x] My code follows the code style of this project and passes `yarn run lint`.
- [x] My change has adequate [Unit Test coverage](https://publish.obsidian.md/tasks-contributing/Testing/About+Testing).

## Terms

- [x] My contribution follow this project's [contributing guide](https://publish.obsidian.md/tasks-contributing)
- [x] I agree to follow this project's [Code of Conduct](https://github.com/obsidian-tasks-group/obsidian-tasks/blob/main/CODE_OF_CONDUCT.md)



Patch:
diff --git a/src/TaskFieldRenderer.ts b/src/TaskFieldRenderer.ts
index a3658f23..4d34e48c 100644
--- a/src/TaskFieldRenderer.ts
+++ b/src/TaskFieldRenderer.ts
@@ -3,26 +3,24 @@ import { PriorityTools } from './lib/PriorityTools';
 import type { Task } from './Task';
 import type { TaskLayoutComponent } from './TaskLayout';
 
-export type AttributesDictionary = { [key: string]: string };
-
 export class TaskFieldRenderer {
     private readonly data = taskFieldHTMLData;
 
     /**
-     * Searches for the component among the {@link taskFieldHTMLData} and gets its data attribute
-     * in a given task. The data attribute shall be added in the task's `<span>`.
-     * For example, a task with medium priority and done yesterday will have
-     * `data-task-priority="medium" data-task-due="past-1d" ` in its data attributes.
+     * Adds data attribute to an {@link element} for a component. For example,
+     * a `<span>` describing a task with medium priority and done yesterday will have
+     * `data-task-priority="medium" data-task-due="past-1d"` in its data attributes (One data attribute per component).
      *
-     * If the data attribute is absent in the task, an empty {@link AttributesDictionary} is returned.
+     * If no data was found for a component in a task, data attribute won't be added.
      *
-     * For detailed calculation see {@link TaskFieldHTMLData.dataAttribute}.
+     * For detailed calculation see {@link TaskFieldHTMLData.addDataAttribute}.
      *
-     * @param component the component of the task for which the data attribute has to be generated.
-     * @param task the task from which the data shall be taken
+     * @param element the HTML element to add the data attribute to.
+     * @param task the task from which the for the data attribute shall be taken.
+     * @param component the component of the task for which the data attribute has to be added.
      */
-    public dataAttribute(component: TaskLayoutComponent, task: Task) {
-        return this.data[component].dataAttribute(component, task);
+    public addDataAttribute(element: HTMLElement, task: Task, component: TaskLayoutComponent) {
+        this.data[component].addDataAttribute(element, task, component);
     }
 
     /**
@@ -107,25 +105,22 @@ export class TaskFieldHTMLData {
     }
 
     /**
-     * Shall be called only by {@link TaskFieldRenderer}. Use that class if you need the data attributes.
+     * Shall be called only by {@link TaskFieldRenderer}. Use that class if you need to add a data attribute.
      *
-     * @returns the data attribute, associated to with a task's component, added in the task's `<span>`.
+     * Adds the data attribute, associated to with a task's component to an HTML element.
      * For example, a task with medium priority and done yesterday will have
-     * `data-task-priority="medium" data-task-due="past-1d" ` in its data attributes.
+     * `data-task-priority="medium" data-task-due="past-1d" ` in its data attributes (One data attribute per component).
      *
      * Calculation of the value is done with {@link TaskFieldHTMLData.attributeValueCalculator}.
      *
-     * @param component the component of the task for which the data attribute has to be generated.
-     * @param task the task from which the data shall be taken
+     * @param element the HTML element to add the data attribute to.
+     * @param task the task from which the data shall be taken.
+     * @param component the component of the task for which the data attribute has to be added.
      */
-    public dataAttribute(component: TaskLayoutComponent, task: Task) {
-        const dataAttribute: AttributesDictionary = {};
-
+    public addDataAttribute(element: HTMLElement, task: Task, component: TaskLayoutComponent) {
         if (this.attributeName !== TaskFieldHTMLData.noAttributeName) {
-            dataAttribute[this.attributeName] = this.attributeValueCalculator(component, task);
+            element.dataset[this.attributeName] = this.attributeValueCalculator(component, task);
         }
-
-        return dataAttribute;
     }
 }
 
diff --git a/src/TaskLineRenderer.ts b/src/TaskLineRenderer.ts
index a50cf38a..7b379e45 100644
--- a/src/TaskLineRenderer.ts
+++ b/src/TaskLineRenderer.ts
@@ -68,11 +68,9 @@ export class TaskLineRenderer {
     }
 
     /**
-     * Renders a given Task object into an HTML List Item (LI) element, using the given renderDetails
-     * configuration and a supplied TextRenderer (typically the Obsidian Markdown renderer, but for testing
-     * purposes it can be a simpler one).
+     * Renders a given Task object into an HTML List Item (LI) element.
      *
-     * The element includes the task and its various components (description, priority, block link etc), the
+     * The element includes the task and its various components (description, priority, block link etc.), the
      * checkbox on the left with its event handling of completing the task, and the button for editing the task.
      *
      * @returns an HTML rendered List Item element (LI) for a task.
@@ -158,7 +156,7 @@ export class TaskLineRenderer {
                     // Inside that text span, we are creating another internal span, that will hold the text itself.
                     // This may seem redundant, and by default it indeed does nothing, but we do it to allow the CSS
                     // to differentiate between the container of the text and the text itself, so it will be possible
-                    // to do things like surrouding only the text (rather than its whole placeholder) with a highlight
+                    // to do things like surrounding only the text (rather than its whole placeholder) with a highlight
                     const internalSpan = document.createElement('span');
                     span.appendChild(internalSpan);
                     await this.renderComponentText(internalSpan, componentString, component, task);
@@ -169,27 +167,24 @@ export class TaskLineRenderer {
                     span.classList.add(...[componentClass]);
 
                     // Add the component's attribute ('priority-medium', 'due-past-1d' etc.)
-                    const componentDataAttribute = fieldRenderer.dataAttribute(component, task);
-                    for (const key in componentDataAttribute) span.dataset[key] = componentDataAttribute[key];
-                    for (const key in componentDataAttribute) li.dataset[key] = componentDataAttribute[key];
+                    fieldRenderer.addDataAttribute(span, task, component);
+                    fieldRenderer.addDataAttribute(li, task, component);
                 }
             }
         }
 
         // Now build classes for the hidden task components without rendering them
         for (const component of taskLayout.hiddenTaskLayoutComponents) {
-            const hiddenComponentDataAttribute = fieldRenderer.dataAttribute(component, task);
-            for (const key in hiddenComponentDataAttribute) li.dataset[key] = hiddenComponentDataAttribute[key];
+            fieldRenderer.addDataAttribute(li, task, component);
         }
 
-        // If a task has no priority field set, its priority will not be rendered as part of the loop above and
+        // If a task has no priority field set, its priority will not be rendered as part of the loop above, and
         // it will not be set a priority data attribute.
         // In such a case we want the upper task LI element to mark the task has a 'normal' priority.
         // So if the priority was not rendered, force it through the pipe of getting the component data for the
         // priority field.
         if (li.dataset.taskPriority === undefined) {
-            const priorityDataAttribute = fieldRenderer.dataAttribute('priority', task);
-            for (const key in priorityDataAttribute) li.dataset[key] = priorityDataAttribute[key];
+            fieldRenderer.addDataAttribute(li, task, 'priority');
         }
     }
 
@@ -249,14 +244,14 @@ export class TaskLineRenderer {
      */
     private addInternalClasses(component: TaskLayoutComponent, internalSpan: HTMLSpanElement) {
         /*
-         * Sanitize tag names so they will be valid attribute values according to the HTML spec:
+         * Sanitize tag names, so they will be valid attribute values according to the HTML spec:
          * https://html.spec.whatwg.org/multipage/parsing.html#attribute-value-(double-quoted)-state
          */
         function tagToAttributeValue(tag: string) {
             // eslint-disable-next-line no-control-regex
             const illegalChars = /["&\x00\r\n]/g;
             let sanitizedTag = tag.replace(illegalChars, '-');
-            // And if after sanitazation the name starts with dashes or underscores, remove them.
+            // And if after sanitization the name starts with dashes or underscores, remove them.
             sanitizedTag = sanitizedTag.replace(/^[-_]+/, '');
             if (sanitizedTag.length > 0) return sanitizedTag;
             else return null;


Test patch:
diff --git a/tests/TaskFieldRenderer.test.ts b/tests/TaskFieldRenderer.test.ts
index 0e52a819..f376e998 100644
--- a/tests/TaskFieldRenderer.test.ts
+++ b/tests/TaskFieldRenderer.test.ts
@@ -7,6 +7,8 @@ import { TaskBuilder } from './TestingTools/TaskBuilder';
 
 window.moment = moment;
 
+const fieldRenderer = new TaskFieldRenderer();
+
 describe('Field Layouts Container tests', () => {
     beforeEach(() => {
         jest.useFakeTimers();
@@ -17,51 +19,52 @@ describe('Field Layouts Container tests', () => {
         jest.useRealTimers();
     });
 
-    it('should get the data attribute of an existing component (date)', () => {
-        const container = new TaskFieldRenderer();
+    it('should add a data attribute for an existing component (date)', () => {
         const task = new TaskBuilder().dueDate('2023-11-20').build();
+        const span = document.createElement('span');
 
-        const dueDateDataAttribute = container.dataAttribute('dueDate', task);
+        fieldRenderer.addDataAttribute(span, task, 'dueDate');
 
-        expect(Object.keys(dueDateDataAttribute).length).toEqual(1);
-        expect(dueDateDataAttribute['taskDue']).toEqual('future-1d');
+        expect(Object.keys(span.dataset).length).toEqual(1);
+        expect(span.dataset['taskDue']).toEqual('future-1d');
     });
 
-    it('should get the data attribute of an existing component (not date)', () => {
-        const container = new TaskFieldRenderer();
+    it('should add a data attribute for an existing component (not date)', () => {
         const task = TaskBuilder.createFullyPopulatedTask();
+        const span = document.createElement('span');
 
-        const dueDateDataAttribute = container.dataAttribute('priority', task);
+        fieldRenderer.addDataAttribute(span, task, 'priority');
 
-        expect(Object.keys(dueDateDataAttribute).length).toEqual(1);
-        expect(dueDateDataAttribute['taskPriority']).toEqual('medium');
+        expect(Object.keys(span.dataset).length).toEqual(1);
+        expect(span.dataset['taskPriority']).toEqual('medium');
     });
-
-    it('should return empty data attributes dictionary for a missing component', () => {
-        const container = new TaskFieldRenderer();
+    it('should not add any data attributes for a missing component', () => {
         const task = new TaskBuilder().build();
+        const span = document.createElement('span');
 
-        const dueDateDataAttribute = container.dataAttribute('recurrenceRule', task);
+        fieldRenderer.addDataAttribute(span, task, 'recurrenceRule');
 
-        expect(Object.keys(dueDateDataAttribute).length).toEqual(0);
+        expect(Object.keys(span.dataset).length).toEqual(0);
     });
 });
 
 describe('Field Layout Detail tests', () => {
-    it('should supply a class name and a data attribute name', () => {
+    it('should supply a class name', () => {
         const fieldLayoutDetail = new TaskFieldHTMLData('stuff', 'taskAttribute', () => {
             return '';
         });
         expect(fieldLayoutDetail.className).toEqual('stuff');
     });
 
-    it('should return a data attribute', () => {
+    it('should add a data attribute for an HTML element', () => {
         const fieldLayoutDetail = new TaskFieldHTMLData('dataAttributeTest', 'aKey', () => {
             return 'aValue';
         });
-        const dataAttribute = fieldLayoutDetail.dataAttribute('description', new TaskBuilder().build());
+        const span = document.createElement('span');
+
+        fieldLayoutDetail.addDataAttribute(span, new TaskBuilder().build(), 'description');
 
-        expect(Object.keys(dataAttribute).length).toEqual(1);
-        expect(dataAttribute['aKey']).toEqual('aValue');
+        expect(Object.keys(span.dataset).length).toEqual(1);
+        expect(span.dataset['aKey']).toEqual('aValue');
     });
 });
diff --git a/tests/TaskLineRenderer.test.ts b/tests/TaskLineRenderer.test.ts
index 58f3092a..dac39fb6 100644
--- a/tests/TaskLineRenderer.test.ts
+++ b/tests/TaskLineRenderer.test.ts
@@ -8,7 +8,7 @@ import { resetSettings, updateSettings } from '../src/Config/Settings';
 import { DateParser } from '../src/Query/DateParser';
 import type { Task } from '../src/Task';
 import { TaskRegularExpressions } from '../src/Task';
-import { type AttributesDictionary, TaskFieldRenderer } from '../src/TaskFieldRenderer';
+import { TaskFieldRenderer } from '../src/TaskFieldRenderer';
 import { LayoutOptions } from '../src/TaskLayout';
 import type { TextRenderer } from '../src/TaskLineRenderer';
 import { TaskLineRenderer } from '../src/TaskLineRenderer';
@@ -19,6 +19,8 @@ import { TaskBuilder } from './TestingTools/TaskBuilder';
 jest.mock('obsidian');
 window.moment = moment;
 
+type AttributesDictionary = { [key: string]: string };
+
 const fieldRenderer = new TaskFieldRenderer();
 
 /**