Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add logger warnings when slots override attributes in NodeProcessor.ts #2476

Closed
4 tasks done
luminousleek opened this issue Mar 26, 2024 · 2 comments · Fixed by #2511 or #2525
Closed
4 tasks done

Add logger warnings when slots override attributes in NodeProcessor.ts #2476

luminousleek opened this issue Mar 26, 2024 · 2 comments · Fixed by #2511 or #2525

Comments

@luminousleek
Copy link
Contributor

luminousleek commented Mar 26, 2024

Please confirm that you have searched existing issues in the repo

Yes, I have searched the existing issues

Any related issues?

#2463, #1715

What is the area that this feature belongs to?

Testing

Is your feature request related to a problem? Please describe.

There are two issues that arise from #2463 (and I'll be quoting myself):

Inconsistent naming in NodeProcessor.data.ts

The naming conventions of the test inputs in NodeProcessor.data.ts are inconsistent. Many of the elements there have a test to check if the slots do override the attributes of the elements, but they are named in two separate ways:

  1. PROCESS_[ELEMENT]_ATTRIBUTES_NO_OVERRIDE
  2. PROCESS_[ELEMENT]_[ATTRIBUTE]_SLOT_TAKES_PRIORITY

I think that's why the author of #2053 wrote another constant called PROCESS_PANEL_HEADER_SLOT_TAKES_PRIORITY when the constant PROCESS_PANEL_ATTRIBUTES_NO_OVERRIDE tests for the exact same thing. That's why I have an expect statement after each of these two tests, because in both times the logger is called with the same input.

Logger warnings still inconsistent

With the merging of #2053, Panels, Dropdowns, Modals and Popovers print warnings to the console if there are slots overriding the element attributes. However, other elements such as Quizzes and QOptions do not print warnings when this happens. This is because in the code of MdAttributeRenderer.ts, not every attribute is checked to see if there is an overriding slot, so the logger will not warn if the unchecked attributes are overridden.

Describe the solution you'd like

Firstly, standardise the constant names in NodeProcessor.data.ts to PROCESS_[ELEMENT]_[ATTRIBUTE]_SLOT_TAKES_PRIORITY as it is clearer than the other alternative.

To fix the inconsistent logger warnings once and for all, implement this new method

processAttribute(node, attribute, isInline, slotName = attribute) {
    if (!this.hasSlotOverridingAttribute(node, attribute, slotName) {
        this.processAttributeWithoutOverride(node, attribute, isInline, slotName);
    }
}

and use it for every attribute of the node.

Lastly, create unit tests so for every attribute that can potentially be overriden.

This issue will require at least 3 PRs, namely:

Describe alternatives you've considered

No response

Additional context

No response

@luminousleek
Copy link
Contributor Author

For reference, these are the components and attributes that can be overridden by slots that do not have unit tests for them:

Component Attribute
Modal Header
Box Icon, Header
Scroll-top-button Icon

@kaixin-hc
Copy link
Contributor

Bringing over the summary from #2511 here - feel free to edit this message to bring it up to date. I might have missed something - let me know!

Referring to packages/core/test/unit/html/NodeProcessor.test.ts

Component Attributes Documented? Tested?
Panel Header, Alt Yes(?) (Alt seems to not be tested?)
Box Icon, Header ? Not tested
Popover Header, Content Yes Yes
Modal Header ? Not tested
Dropdown Header ? Yes
Scroll-top-button Icon ? Not tested
Question Header, Hint, Answer Yes Yes
Q-Option Reason Yes Yes
Quiz Intro Yes Yes
Tooltip Content ? Not tested
Tab Header ? Not tested
A-Point Header, Content, Label No Not tested

So when adding tests for these 6 not tested parts we should also update docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment