refactor: do not add data attribute when it's value is empty

[ilandikov]

Description

After refactoring tests, a bug has been revealed (it was present before, but unseen due to the way the tests were done).

Current behaviour: data attributes with empty values are added.
Fixed behaviour: data attributes with empty values are not added.

Motivation and Context

Fix bugs =)

How has this been tested?

Unit tests.

Types of changes

Changes visible to users:

• Bug fix (prefix: fix - non-breaking change which fixes an issue)

Checklist

• My code follows the code style of this project and passes yarn run lint.
• My change has adequate Unit Test coverage.

Terms

• My contribution follow this project's contributing guide
• I agree to follow this project's Code of Conduct

[claremacrae]

Fix bugs =)

What are the steps to reproduce the bug(s) please? How is it visible to users?

[claremacrae]

Fix bugs =)

What are the steps to reproduce the bug(s) please? How is it visible to users?

Asking because:

• I wish to understand if it’s a theoretical bug or an actual bug that can be created from real user task data
• I wish to know how to write it up in the next release notes

[ilandikov]

What are the steps to reproduce the bug(s) please? How is it visible to users?

Well, I'm in doubt here. I'm not sure how/whether the user can see this. Let me provide you a bit more context. Previously the test for test attributes were written like:

const testComponentClasses = async (
        taskLine: string,
        layoutOptions: Partial<LayoutOptions>,
        mainClass: string,
        attributes: AttributesDictionary,
    ) => {
        // skipping unrelated code

        // Now verify the attributes
        for (const key in attributes) {
            expect(listItem.dataset[key]).toEqual(attributes[key]);
        }

and now take a look at the call:

await testComponentClasses(
            '- [ ] Full task ⏫ 📅 2022-07-02 ⏳ 2022-07-03 🛫 2022-07-04 🔁 every day',
            {},
            fieldRenderer.className('recurrenceRule'),
            {},
        );

The attributes in an empty dictionary so the loop for (const key in attributes) is not run at all, which means that the test is not testing the absence of the data attributes. Now the tests are actually converting the data attribute to a string and comparing them as string. So if the attributes are empty, the matcher will check that.

[ilandikov]

Asking because:

• I wish to understand if it’s a theoretical bug or an actual bug that can be created from real user task data
• I wish to know how to write it up in the next release notes

Perfectly valid questions. I would say it is more of a theoretical internal bug

[claremacrae]

Thanks for the replies.
So yes, I think this is likely:

1. a "refactor" of code in src/ - no difference in user-observable behaviour...
2. a "fix" of test behaviour (from the description above...)

[ilandikov]

Thanks for the replies. So yes, I think this is likely:

1. a "refactor" of code in src/ - no difference in user-observable behaviour...
2. a "fix" of test behaviour (from the description above...)

Ah ok, I will take note for that =) I had a thought like make tests not fail -> fix =)

I see you closed the PR, would you like me to submit with a different branch starting with refactor-... or something you are not interested in this code?

[claremacrae]

I see you closed the PR

Thanks for pointing that out. It was certainly not my intention... 😢

[claremacrae]

There's basically a dividing line in the release notes in to two halves...

Almost aways, all the fix and feat are in the top half, as relevant to users....

This is done by me manually editing the PR text to group related items together...

---

The 5.3.0 release was trickier as a bunch of fixes and features were made, on a feature not yet released - and so those were not relevant to users...

This is what I ended up doing:

[ilandikov]

Ok, clear, thanks =)

[claremacrae]

I'm trying to find a reference to why it is wrong for a data attribute to not have a value.

And not succeeding.

https://stackoverflow.com/questions/56957757/are-empty-boolean-html-attributes-the-same-as-no-value-attributes
https://stackoverflow.com/questions/9729080/are-empty-html-data-attributes-valid

[claremacrae]

I'm trying to find a reference to why it is wrong for a data attribute to not have a value.

And not succeeding.

https://stackoverflow.com/questions/56957757/are-empty-boolean-html-attributes-the-same-as-no-value-attributes https://stackoverflow.com/questions/9729080/are-empty-html-data-attributes-valid

Asking as, if it's not a user-visible issue right now, as there is no current scenario where users can create empty data attribute values - and there is no technical reason to prevent them - might this actually create a pitfall for our future selves, if a future styling development might actually want to allow empty data attributes in future?

[ilandikov]

Please note that in the examples above, the questions were asked for data attributes with boolean types, where an attribute’s presence equals attribute presence with empty value, equals attribute presence with “true” value. Here we are talking about data attributes with actual values, they don’t have a sense at all without a value.

My feeling is that the tests were written with total absence of attributes in the HTMLElements.

Asking as, if it's not a user-visible issue right now, as there is no current scenario where users can create empty data attribute values - and there is no technical reason to prevent them - might this actually create a pitfall for our future selves, if a future styling development might actually want to allow empty data attributes in future?

Good question. I would say that 1) YAGNI ;) 2) it will be quick to change in this case.

One more thing, my objective is to remove the ‘it.failing()’ tests. The other way could be to change the expected value in the tests. Pls lmk if you would prefer this solution.

[claremacrae]

One more thing, my objective is to remove the ‘it.failing()’ tests. The other way could be to change the expected value in the tests. Pls lmk if you would prefer this solution.

Indeed.

My concern is exactly that I do not understand the intention of the code, so am unable to say whether the correct answer is to change the tests or to change the code.

I will go ahead and merge the PR, as you clearly know more about how this code works than I do.

[claremacrae]

Here we are talking about data attributes with actual values, they don’t have a sense at all without a value.

That's helpful. Thanks.

[claremacrae]

I ran the styling smoke test, as the PR changed production code...
It of course passed.

[ilandikov]

Thanks~~