Inconsistent logger warning for repeated slots/attributes

[jovyntls]

Tell us about your environment
MacOS 12.0.1

• MarkBind Version: 3.1.1

What did you do? Please include the actual source code causing the issue.

• Create a dropdown component with the same slot and attribute.
• Create any other component with the same slot and attribute

(Source code causing the issue: link)

E.g.

### Dropdowns
This triggers a logger warning:
<dropdown header="Does not appear, overwritten by slot">
  <span slot="header">Dropdown header slot</span>
  <li><a href="#dropdown" class="dropdown-item">Lorem ipsum</a></li>
</dropdown>

### Panels
This **does not** trigger a warning:
<panel header="Does not appear, overwritten by slot">
  <span slot="header">Panel header slot</span>
  Lorem ipsum
</panel>

### Popover
This **does not** trigger a warning:
<popover header="Does not appear, overwritten by slot" content="Does not appear">
  <span slot="header">Popover header slot</span>
  <span slot="content">Popover content slot</span>
  Lorem ipsum
</popover>

What did you expect to happen?
Repeating an attribute as a slot either always triggers a warning, or never triggers a warning.

What actually happened? Please include the actual, raw output.
A warning is triggered for dropdowns, but not for any other component.

Dropdowns has some code that checks against having both the header slot and header attribute, and logs a warning.

I'm wondering if there is a reason why Dropdowns have this specific logger warning, but not any other component?
I tried removing the check for the duplicated slot/attribute in Dropdowns to check if this code exists because failing to delete the header attribute will cause issues, but it does not seem like the case, as Dropdowns continue to work correctly without the check.

[jonahtanjz]

Seems like this was introduced in #1057 to address this issue #1048:

• All components
  • warn about deprecated attributes
  • warn about inconsistent slot + attribute ( that also translates to the slot ) choices ( e.g. using content attribute and content slot for popovers at the same time )

Somehow popovers and panels were missed out? I think we can add in the warnings for both the components to make it consistent.

[lhw-1]

(I am a newcomer, so if there are any issues with my approach / testing, please let me know.)

I saw that this issue was still open, so I have tested this on Ubuntu 20.04 with MarkBind v4.0.2. I tried generating a website with this exact code given as example above:

### Dropdowns
This triggers a logger warning:
<dropdown header="Does not appear, overwritten by slot">
  <span slot="header">Dropdown header slot</span>
  <li><a href="#dropdown" class="dropdown-item">Lorem ipsum</a></li>
</dropdown>

### Panels
This **does not** trigger a warning:
<panel header="Does not appear, overwritten by slot">
  <span slot="header">Panel header slot</span>
  Lorem ipsum
</panel>

### Popover
This **does not** trigger a warning:
<popover header="Does not appear, overwritten by slot" content="Does not appear">
  <span slot="header">Popover header slot</span>
  <span slot="content">Popover content slot</span>
  Lorem ipsum
</popover>

I expected only the dropdown to display a warning for the repeated attribute, as mentioned in the initial issue reporting.

This is what I got:

Not only did it display warnings correctly for the popover element, it completely glossed over the panel element, different from what was mentioned in the initial issue reporting. It looks like the warnings for the dropdown and popover elements have been implemented correctly since (respectively link and link), with warnings for popover elements included in #1780, but not for the panel element (link).

Solving the warning issue was simple enough; add in the hasSlotOverridingAttribute method into the processPanelAttributes method as can be seen here. This is what I get after the change:

Would it be alright to submit this as a PR for review (trivial as it may be)?

Also, after looking at the code organization in the file as a whole, it seems that there are many instances of processing attributes without overriding. There may be a need to tweak the way warnings are currently handled, e.g. a function that logs a warning given any element / attribute, instead of having to create new handlers for each element and running the method for each attribute. It would be great to get some feedback on this!

[jovyntls]

Thanks for tracking down where the changes were made!

There may be a need to tweak the way warnings are currently handled, e.g. a function that logs a warning given any element / attribute, instead of having to create new handlers for each element and running the method for each attribute. It would be great to get some feedback on this!

I think this is a good idea! It would be great to handle slots and attributes in a way that throws a warning when an override occurs, without us having to have a specific handler for each case (as we currently have). If you're doing so, you might want to take note of possible edge cases with slots/attributes (e.g. the expected behaviour for popovers), or cases when the slot and attribute name are different.

If the implementation would be too convoluted then it's fine to just add the warning for panels like you have :)

Would it be alright to submit this as a PR for review (trivial as it may be)?

Yup please go ahead! It's not trivial 🙂