Implement method to process attributes that can be overridden by slots #2511

luminousleek · 2024-04-10T08:06:54Z

What is the purpose of this pull request?

Partially resolves #2476

Overview of changes:

Implement processSlotAttribute method and use it for attributes that can be overridden by slots.

Anything you'd like to highlight/discuss:

For reference, these are the attributes that can be overridden by slots:

Component	Attributes
Panel	Header, Alt
Box	Icon, Header
Popover	Header, Content
Modal	Header
Dropdown	Header
Scroll-top-button	Icon
Question	Header, Hint, Answer
Q-Option	Reason
Quiz	Intro
Tooltip	Content
Tab	Header
A-Point	Header, Content, Label

The docs are not correct regarding this.

Testing instructions:

Proposed commit message: (wrap lines at 72 characters)

Implement processSlotAttribute method

There is some duplicated code to process attributes that can
be overridden by slots. In addition, not every attribute that can be
overridden with slots are processed with the 
hasSlotOverridingAttribute method. As such, there will be no
warning in the logger when these attributes are overridden.

Let's put this duplicated code into a method and call it for every
attribute that can be overridden by a slot.

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features (will do in the next PR!)
Linked all related issues
No unrelated changes

Reviewer checklist:

Indicate the SEMVER impact of the PR:

Major (when you make incompatible API changes)
Minor (when you add functionality in a backward compatible manner)
Patch (when you make backward compatible bug fixes)

At the end of the review, please label the PR with the appropriate label: r.Major, r.Minor, r.Patch.

Breaking change release note preparation (if applicable):

To be included in the release note for any feature that is made obsolete/breaking

Give a brief explanation note about:

what was the old feature that was made obsolete

any replacement feature (if any), and

how the author should modify his website to migrate from the old feature to the replacement feature (if possible).

and use it for attributes that can be overridden by slots

luminousleek · 2024-04-10T08:12:13Z

packages/core/src/html/MdAttributeRenderer.ts

+  }
+
+  processPopoverAttributes(node: MbNode) {
+    this.processSlotAttribute(node, 'header', true);

    // Warn if there is a content slot overriding the attributes 'content' or 'src'
    const hasSlotAndContentAttribute = this.hasSlotOverridingAttribute(node, 'content', 'content');


This bit was left unchanged because the logic for the content attribute is slightly different. According to the docs, the srcand content attributes and the content slot can be used for popovers. If all three are present then MarkBind will prioritise them in the following order:

content slot

content attribute

src attribute

So the logic to process this attribute (and especially the logger warning) is different than the other attributes. Specifically, it needs to check for the case that the src and content attributes are both used, and show a warning message if that is the case. The processSlotAttribute method does not account for this case, so this chunk of code is still here.

luminousleek · 2024-04-10T08:13:50Z

packages/core/src/html/MdAttributeRenderer.ts

+   * @param isInline Whether to process the attribute with only inline markdown-it rules
+   * @param slotName Name attribute of the <slot> element to insert, which defaults to the attribute name
+   */
+  processSlotAttribute(node: MbNode, attribute: string, isInline: boolean, slotName = attribute) {


I'm not entirely happy with the name of the method but it's the best I can come up with. Some other alternatives I've considered are:

processSlottableAttribute

processAttributeCanBeOverridden

processOverridableAttribute

processAttributeWithSlot

Let me know your thoughts thanks.

I think processSlotAttribute is ok!

luminousleek · 2024-04-10T08:44:48Z

The check_docs test is failing because the PR is doing its job - it's giving logger warnings when attributes for a quiz are being overridden:

The warnings arise from the panel in the Header and Hint example with slots at this page (the relevant documentation file is /docs/userGuide/syntax/questions.md), where the code for the question in the panel is

<question type="checkbox" header="Which of the following is true?" hint="Think out of the box! :fas-box:">
  <!-- Header slot -->
  <div slot="header">
<p>Which of the following is correct?</p>
<p>Challenge: Try to get all the answers on your first try! ⭐️ ⭐️</p>
  </div>
<!-- other code omitted for brevity -->
  <!-- Hint slot -->
  <div slot="hint">
<p>Think out of the box! <span aria-hidden="true" class="fas fa-box"></span></p>
<p>Need another hint? <tooltip content="Two of the answers are correct!">Hover over me!</tooltip> <span aria-hidden="true" class="fas fa-mouse-pointer"></span></p>
  </div>
</question>

As can be seen, the header and hint attributes are overridden by their respective slots, which then triggers a logger warning. A simple fix would be to delete the header and hint attributes, so I'm wondering if it's OK to do this in this PR or if I need to create a separate PR for this.

yucheng11122017

Thank you for the detailed PR @luminousleek!

I realized that our documentation might not be fully up to date - other attributes seem to work with a slot as well (like a-point) but it doesn't seem to be present in our documentation.
I think we might need to double check this behavior and update the documentation

yucheng11122017 · 2024-04-12T01:35:03Z

packages/core/src/html/MdAttributeRenderer.ts

+   * @param isInline Whether to process the attribute with only inline markdown-it rules
+   * @param slotName Name attribute of the <slot> element to insert, which defaults to the attribute name
+   */
+  processSlotAttribute(node: MbNode, attribute: string, isInline: boolean, slotName = attribute) {


I think processSlotAttribute is ok!

packages/core/src/html/MdAttributeRenderer.ts

luminousleek · 2024-04-12T05:42:20Z

I tested the rest of the other attributes for the components to see whether they're able to be overridden by slots.

Looks like the header, content and label attributes for a-point are able to be overridden by slots

Also, the header attribute for tab is also able to be overridden by a slot

As well as the content attribute for tooltip

And the alt attribute for panel

Basically that's all the attributes that are handled by MdAttributeRenderer.ts. The PR is updated to use processSlotAttributes for the rest of the attributes.

The docs currently has a question where there are slots overriding the header attributes. However, when #2511 is merged, logger warnings about this overriding will be triggered, leading to the check-docs GitHub action failing. Let's remove the header attributes so that they will not trigger the logger warning. Let's also update the sample code to reflect this change.

kaixin-hc

Great catch. I think we can leave the documentation for another PR or concurrent with adding tests for the ones that aren't tested yet.

I like how this makes the code overall clearer by using a simpler method for everything, thanks for your work!

The docs currently has a question where there are slots overriding the header attributes. However, when MarkBind#2511 is merged, logger warnings about this overriding will be triggered, leading to the check-docs GitHub action failing. Let's remove the header attributes so that they will not trigger the logger warning. Let's also update the sample code to reflect this change.

github-actions · 2024-04-14T16:13:41Z

@kaixin-hc Each PR must have a SEMVER impact label, please remember to label the PR properly.

luminousleek added 2 commits April 10, 2024 15:51

implement processSlotAttribute

08c6b18

and use it for attributes that can be overridden by slots

fix lint

01de626

luminousleek commented Apr 10, 2024

View reviewed changes

This was referenced Apr 10, 2024

Overridden Attribute for Question in Documentation causes Logger Warnings and Build Failure #2512

Closed

Remove Overridden Question Attributes in Documentation #2513

Merged

yucheng11122017 reviewed Apr 12, 2024

View reviewed changes

use processSlotAttribute for the rest

80c8545

use slotAttribute for panel alt

a96d0d9

Merge branch 'master' into process-slot-attributes

57ab170

kaixin-hc approved these changes Apr 12, 2024

View reviewed changes

kaixin-hc mentioned this pull request Apr 12, 2024

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

Closed

4 tasks

kaixin-hc merged commit ff1a738 into MarkBind:master Apr 14, 2024
7 checks passed

kaixin-hc added the r.Patch Version resolver: increment by 0.0.1 label Apr 14, 2024

yucheng11122017 mentioned this pull request Apr 25, 2024

Some attributes in documentation does not have information letting users know about slots #2516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement method to process attributes that can be overridden by slots #2511

Implement method to process attributes that can be overridden by slots #2511

luminousleek commented Apr 10, 2024 •

edited by kaixin-hc

Loading

luminousleek Apr 10, 2024

luminousleek Apr 10, 2024

yucheng11122017 Apr 12, 2024

luminousleek commented Apr 10, 2024

yucheng11122017 left a comment

yucheng11122017 Apr 12, 2024

luminousleek commented Apr 12, 2024

kaixin-hc left a comment

github-actions bot commented Apr 14, 2024

Implement method to process attributes that can be overridden by slots #2511

Implement method to process attributes that can be overridden by slots #2511

Conversation

luminousleek commented Apr 10, 2024 • edited by kaixin-hc Loading

luminousleek Apr 10, 2024

Choose a reason for hiding this comment

luminousleek Apr 10, 2024

Choose a reason for hiding this comment

yucheng11122017 Apr 12, 2024

Choose a reason for hiding this comment

luminousleek commented Apr 10, 2024

yucheng11122017 left a comment

Choose a reason for hiding this comment

yucheng11122017 Apr 12, 2024

Choose a reason for hiding this comment

luminousleek commented Apr 12, 2024

kaixin-hc left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 14, 2024

luminousleek commented Apr 10, 2024 •

edited by kaixin-hc

Loading