Include repeated attributes warning for panel elements

[lhw-1]

What is the purpose of this pull request?

• Documentation update
• Bug fix
• Feature addition or enhancement
• Code maintenance
• DevOps
• Improve developer experience
• Others, please explain:

Overview of changes:
Resolves #1764
Related to #1780 which partially resolved this issue but was undocumented

Anything you'd like to highlight/discuss:
While this pull request resolves the original issue of repeated attributes in panel elements not logging a warning message, there may be additional work required to streamline the process of adding warnings instead of declaring a warning case for every element as is currently being done: link

Three further issues have been proposed thus far:

1. Streamline the process of adding warnings as opposed to declaring a warning for each attribute of every element that requires a warning, as highlighted above.
2. Implement a way to have unit tests for logging output from the winston logger. This will allow us to have unit tests for warnings and log outputs as well, which should increase robustness of the system as a whole.
3. Following (2), unit / functional tests for warnings (for the rest of the elements with warnings) should be written.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

Include repeated attributes warning for panel elements

---

Checklist: ☑️

• Updated the documentation for feature additions and enhancements
• Added tests for bug fixes or features
• Linked all related issues
• No unrelated changes

[tlylt]

Hi @lhw-1, thank you for contributing!

Will you be adding unit tests and functional tests for this bug fix? Would be great to have them in place to keep this bug in check. (You can get more information on our testing practices here)

[lhw-1]

@tlylt Thanks for the heads up! I will add in some tests for this issue.

[lhw-1]

Looks like relevant unit tests for panel elements have been previously added under packages/core/test/unit/html/NodeProcessor.data.js (here), and packages/core/test/unit/html/NodeProcessor.test.js (here).

Functional tests for panel elements have been added in commit c489112.

[tlylt]

Looks like relevant unit tests for panel elements have been previously added under packages/core/test/unit/html/NodeProcessor.data.js (here), and packages/core/test/unit/html/NodeProcessor.test.js (here).

Functional tests for panel elements have been added in commit c489112.

If suitable, you can add tests to the existing test files related to the panel. Have you added new test cases specifically for triggering/not triggering the repeated attributes warning? (Possibly with relevant text mentioning what's being tested for the test cases)

[lhw-1]

Have you added new test cases specifically for triggering/not triggering the repeated attributes warning? (Possibly with relevant text mentioning what's being tested for the test cases)

I could not find test cases specifically for triggering / not triggering the repeated attributes warnings for the Popover / Dropdown elements (warnings for these elements have been previously implemented) after a quick search, but I could have missed them, so I'll give it a more thorough search!

On another note, the logger currently uses the winston logger to output such warnings for elements, but I did not find any test cases testing the winston logger (again, I could have missed them... 😅). If so, it may be worthwhile to look into implementing test cases for the logger, which seems reasonably feasible (but out of scope for this particular pull request, so I'll open another issue for this if this happens to be the case).

[tlylt]

I could not find test cases specifically for triggering / not triggering the repeated attributes warnings for the Popover / Dropdown elements (warnings for these elements have been previously implemented) after a quick search, but I could have missed them, so I'll give it a more thorough search!

Probably means they are not written, can file an issue for those separately.

On another note, the logger currently uses the winston logger to output such warnings for elements, but I did not find any test cases testing the winston logger (again, I could have missed them...). If so, it may be worthwhile to look into implementing test cases for the logger, which seems reasonably feasible (but out of scope for this particular pull request, so I'll open another issue for this if this happens to be the case).

I don't think we have the mechanism to match console output at the moment, but we still want to have the cases captured so that minimally on visual inspection we can see the warnings being logged.

Good suggestion for having such an implementation... yes we can file another issue to take that forward 👍

[lhw-1]

To clarify, the case in packages/core/test/unit/html/NodeProcessor.data.js (here) seems to be the only one that is relevant to this issue under packages/core/test/unit, and there do not seem to be other relevant unit tests specific to warnings implemented for other elements either. Should I add in a statement that specifies that a warning will be thrown if repeated attributes are used under the above test case? Though in that case, I think similar statements should also be added in for all the other elements (which may be out of scope for this pull request).

[jovyntls]

Thank you @lhw-1 , the tests look pretty comprehensive in testing slot/attribute/overrides! Just some comments on the alt slot.

On a side note, we could use more tests for all the features we support with panels (e.g. peek, no-close, etc). That's out of the scope of the PR though so don't worry about it :)

[jovyntls]

Should I add in a statement that specifies that a warning will be thrown if repeated attributes are used under the above test case?
Though in that case, I think similar statements should also be added in for all the other elements (which may be out of scope for this pull request).

Agreed on this, I think for this PR we can add a comment for just the panels. 😃

[tlylt]

To clarify, the case in packages/core/test/unit/html/NodeProcessor.data.js (here) seems to be the only one that is relevant to this issue under packages/core/test/unit, and there do not seem to be other relevant unit tests specific to warnings implemented for other elements either. Should I add in a statement that specifies that a warning will be thrown if repeated attributes are used under the above test case? Though in that case, I think similar statements should also be added in for all the other elements (which may be out of scope for this pull request).

Do those unit tests have their individual cases that they are checking for? If so I think we might want to create another similar one (somewhere below perhaps) with the sole focus on the conditions that will trigger the warning. This might help make tests isolated and avoid unnecessary changes in the future. What do you think, @lhw-1?

[lhw-1]

Do those unit tests have their individual cases that they are checking for? If so I think we might want to create another similar one (somewhere below perhaps) with the sole focus on the conditions that will trigger the warning. This might help make tests isolated and avoid unnecessary changes in the future. What do you think, @lhw-1?

Ah, I see what you mean. Yes, I think that would be a good solution in the meantime as well - I'll get to it. Thank you!

[lhw-1]

I have added unit tests for panels here. It seems that there were actually unit tests for this case but only for dropdown elements here, for future reference.

Once this is good to go & hopefully gets merged, I will open up new issues as mentioned in the "things to highlight / discuss" section above (I've updated it) 👍

[jovyntls]

LGTM! 👍

What you've mentioned under "things to highlight/discuss" sound like good ideas to me as well! Thanks for opening the issues :)

[tlylt]

@all-contributors please add @lhw-1 for code

[allcontributors]

@tlylt

I've put up a pull request to add @lhw-1! 🎉