Fix Tab-Group Header not displayed #2557

gerteck · 2024-06-14T09:31:30Z

What is the purpose of this pull request?

Overview of changes:

Header does not show up for the TabGroup component. This was likely due to the header being passed as a prop instead of a named slot into Dropdown. ~~To fix this, pass header as named slot.~~ To fix this, the header is now treated as a prop.

Anything you'd like to highlight/discuss:

From what I understand:

TabGroup internally consists of the dropdown tab component. However, internally, TabGroup does not render this dropdown, but instead adds itself to the parent TabSet, which is then in charge of rendering children Tab or TabGroup.
In TabSet, the header was originally passed into Dropdown from Tabset as an attribute / vue prop of Dropdown. Dropdown then uses this prop to display the header:
- commit history
After that, to add markdown support to the header, the header was passed using slots. I would like to see if this is the source of the bug, but at this point, I can't seem to test this version of MarkBind, due to depreciated dependencies/errors faced.
- commit history

[ Requesting Comments ]: I don't quite understand how the Vue component's prop is being passed as a named slot. Referring to the developer guide, I tried to trace the processing but I think I might be facing a skill issue. I'm confused as it says that "MarkBind attributes are passed to the Vue component as props. The type of the prop will be a String." However, the prop is able to be processed as a named slot.

This seems to be the core of the issue behind the bug. In Tabset, the header is being passed into dropdown similarly as a prop, which is not registered as a namedSlot, and ends up as an attribute when rendered. ~~To fix this, I directly passed the header as a named slot. However the discrepancy and origin of the bug leaves me uneasy.~~

Illustration

Sample Code in MarkBind Syntax (where header is passed as an attribute):

<!-- header is being passed as a prop -->
<dropdown header="Prop Header" class="w-100"> 
    <ul slot="dropdown-menu" class="dropdown-menu">
      <li><a href="#dropdown" class="dropdown-item">Action</a></li>
      <li><a href="#dropdown" class="dropdown-item">Separated link</a></li>
    </ul>
  </dropdown>

Dropdown Vue Component Template (where header is accepted as a named slot):

markbind/packages/vue-components/src/Dropdown.vue

Lines 28 to 51 in 2c82764

    
           <div 
        
             v-else 
        
             ref="dropdown" 
        
             :class="[{ 'disabled': disabledBool }, 'btn-group', addClass]" 
        
           > 
        
             <slot name="before"></slot> 
        
             <slot name="button"> 
        
               <button 
        
                 type="button" 
        
                 class="btn dropdown-toggle" 
        
                 :class="[btnType, btnWithBefore, { 'dropdown-toggle-split': hasBefore }]" 
        
                 :disabled="disabledBool" 
        
                 data-bs-reference="parent" 
        
                 data-bs-toggle="dropdown" 
        
               > 
        
                 <slot name="header"></slot> 
        
               </button> 
        
             </slot> 
        
             <slot name="dropdown-menu" :class="[{ 'show': show }, { 'dropdown-menu-end': menuAlignRight }]"> 
        
               <ul class="dropdown-menu" :class="[{ 'show': show }, { 'dropdown-menu-end': menuAlignRight }]"> 
        
                 <slot></slot> 
        
               </ul> 
        
             </slot> 
        
           </div>

Output:

While the header was passed in as a prop, it is parsed as a named slot.

Some references:

Testing instructions:

Refer to user guide: https://markbind.org/userGuide/components/presentation.html#tabs
Refer to PR's localhost user guide: http://127.0.0.1:8080/userGuide/components/presentation.html#tabs

Before:

After:

Proposed commit message: (wrap lines at 72 characters)

Fix TabGroup Header Rendering Issue

The header for the TabGroup component was not displaying
correctly, due to the header being passed as a prop but
treated as a named slot in the Dropdown component.
To fix this, the header is now treated as a prop.

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
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).

lhw-1 · 2024-06-15T16:46:21Z

I don't quite understand how the Vue component's prop is being passed as a named slot. Referring to the developer guide, I tried to trace the processing but I think I might be facing a skill issue. I'm confused as it says that "MarkBind attributes are passed to the Vue component as props. The type of the prop will be a String." However, the prop is able to be processed as a named slot.

The createSlotTemplateNode method is used to convert slots into template nodes directly; this is called by the processAttributeWithoutOverride method, which is in turned called for specified named slots that we are looking out for. This is how we process named slots - hopefully this addresses your question (and if it doesn't, do let me know).

Regardless, this issue might need further investigation 😓

lhw-1 · 2024-06-15T16:52:30Z

packages/vue-components/src/Tabset.vue

+          <template #header>
+            <span v-html="t.headerRendered"></span>
+          </template>


Instead of making this change (since we rely on the conversions from slots to templates, and do not use header templates directly in our .vue files), a suggestion I have is to just directly treat the header as a prop instead, without relying on slot conversion?

In Dropdown.vue, we can first include the header as a prop:

props: { ... header: { type: String, default: '', }, ...

And then we can replace the <slot name="header"></slot> under the v-if, with {{ header }}. We could also rename header to something like tabgroup-header to differentiate it as well.

This might be a less hacky solution, but if we can investigate the issue deeper and find a better solution, that would be best 👍

gerteck · 2024-06-20T04:53:08Z

The createSlotTemplateNode method is used to convert slots into template nodes directly; this is called by the processAttributeWithoutOverride method, which is in turned called for specified named slots that we are looking out for. This is how we process named slots - hopefully this addresses your question (and if it doesn't, do let me know).

Regardless, this issue might need further investigation 😓

It does address my question, thank you! Running it in the debugger with the appropriate breakpoint, I can see it too:

Am I right to say that because dropdown is directly being imported and referenced by Tabset.vue, and consequently is not subjected to the node processing flow, hence it would be wrong to pass the slot as an attribute within the Tabset.vue file? (As you mentioned, that we do not use header templates directly in our .vue files)

If this is the case, I agree with your suggestion to treat header as a prop which was done previously as well. It definitely does seem like a better alternative that fits the bigger picture. 😄

Tim-Siu · 2024-06-22T09:47:04Z

The createSlotTemplateNode method is used to convert slots into template nodes directly; this is called by the processAttributeWithoutOverride method, which is in turned called for specified named slots that we are looking out for. This is how we process named slots - hopefully this addresses your question (and if it doesn't, do let me know).
Regardless, this issue might need further investigation 😓

It does address my question, thank you! Running it in the debugger with the appropriate breakpoint, I can see it too:

Am I right to say that because dropdown is directly being imported and referenced by Tabset.vue, and consequently is not subjected to the node processing flow, hence it would be wrong to pass the slot as an attribute within the Tabset.vue file? (As you mentioned, that we do not use header templates directly in our .vue files)

If this is the case, I agree with your suggestion to treat header as a prop which was done previously as well. It definitely does seem like a better alternative that fits the bigger picture. 😄

Hi @gerteck ,

Thank you for your great research!

I concur with your analysis regarding the root cause of this issue. It appears that the nodes are not subject to recursive processing, and the <dropdown> within Tabset.vue does not undergo transformation by the NodeProcessor to encapsulate the attribute within a <template> wrapper for slot content.

For the immediate solution, I support the two approaches you've outlined:

Manually setting the slot content when importing <dropdown>
Passing the header as a prop

@lhw-1's suggestions appear reasonable, and I recommend adhering to the second approach.

For a more comprehensive solution that might involve modifications to the NodeProcessor (such as recursively processing nodes), we can create a separate issue to address this at a later time when it becomes a priority.

codecov · 2024-06-30T14:09:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 51.86%. Comparing base (2c82764) to head (f92b856).

❗ Current head f92b856 differs from pull request most recent head b1a0055

Please upload reports for the commit b1a0055 to get more accurate results.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2557   +/-   ##
=======================================
  Coverage   51.86%   51.86%           
=======================================
  Files         127      127           
  Lines        5530     5530           
  Branches     1176     1176           
=======================================
  Hits         2868     2868           
  Misses       2367     2367           
  Partials      295      295

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yucheng11122017

Hi @gerteck thanks for the work!
I realised that tabs doesnt have any test cases.. which is why we probably missed out on this regression in behaviour. If you have the time and capacity, you could try writing test cases for tabs. If you don't though, it's ok. Let us know and we will open a issue for this.

yucheng11122017 · 2024-06-30T14:09:09Z

packages/vue-components/src/Dropdown.vue

@@ -11,7 +11,7 @@
        :class="{'disabled': disabledBool}"
        data-bs-toggle="dropdown"
      >
-        <slot name="header"></slot>
+        <span v-html="tabgroupHeader"></span>


Hmm I'm not keen on this naming since this is only true for tab groups but not for other headers.
Could we keep the header name?

Sure! The rationale was give greater differentiation in the way the header is passed as a prop instead of relying on slot conversion, but it is evident from the code directly.

I would be down to try writing some test cases for the tabs! Test cases would also prevent any further regressions until a better way to pass down the header is found.

I just realised that setting it as the same name header causes two of the current tests to fail under dropdown as the header no longer shows up as an attribute in the html rendered.

This is probably because of the slot name and the attribute prop name having the same name.

With that in mind, I think that the tab-group-header prop name should be differentiated from the header slot name as it can get confusing as well.

Tim-Siu · 2024-07-01T03:43:05Z

Hi @gerteck thanks for the work! I realised that tabs doesnt have any test cases.. which is why we probably missed out on this regression in behaviour. If you have the time and capacity, you could try writing test cases for tabs. If you don't though, it's ok. Let us know and we will open a issue for this.

I think we do have some unit test regrading tab-group in NodeProcessor.test.ts but more advanced unit tests or functional test may be necessary to test the intended behavior for the header.

The header named slot is still being used for navbar, hence removing it causes regression for dropdowns under navbars.

To pass tests

gerteck · 2024-07-04T07:15:13Z

Added some snapshot tests for Tabset (Tabs), Tabgroup, Tabs to catch visual regressions during future changes.
- This should fall in the scope of the PR since it addresses the Tab-Group Header display.
Renamed the header prop back into tabGroupHeader, as currently, it is a workaround used only to generate tabgroups in Tabset, and I think it would be confusing to have both a header named slot and prop.
- Noticed a regression related to the DOM structure of nested dropdowns in Dropdown.spec.js if I use the prop header with the same name. Although no visual changes were apparent, the structure was affected. (Failed snapshot testcase) I'm still not sure why after debugging. Hence, going with tabGroupHeader for the name of the prop.
Readded the named slot header
- I missed out a regression when I removed the named slot header. (The impacted use case was navbar with dropdown children in markbind syntax, where the header disappeared).
- It also looks like there are no test cases for this so I will add some as well, possibly in a separate PR.

Any feedback on further streamlining and standardizing these changes is highly welcome!! 😄

Remove unnecessary import

…/markbind into branch-TabGroupHeaderFix

Tim-Siu

Thank you for delving into adding snapshot tests for the Vue plugins. I think they are well-written and sufficient to cover the concern of the issue, i.e., Tab-Group Header not displayed in when Dropdown is nested in Tabset.

github-actions · 2024-07-13T11:16:21Z

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

tlylt · 2024-07-13T11:38:30Z

@Tim-Siu please replace the commit message with the proposed message whenever you merge the PR in the future.

Tim-Siu · 2024-07-13T11:42:43Z

@Tim-Siu please replace the commit message with the proposed message whenever you merge the PR in the future.

Sorry for the mistake. Should I update the commit message and force push?

tlylt · 2024-07-14T15:06:28Z

@Tim-Siu please replace the commit message with the proposed message whenever you merge the PR in the future.

Sorry for the mistake. Should I update the commit message and force push?

Think we can leave it, just take note next time

gerteck added 2 commits June 13, 2024 13:43

Add content to named slot

ab7984b

Remove header prop

624fa5b

lhw-1 reviewed Jun 15, 2024

View reviewed changes

Add props for TabGroup header

f92b856

gerteck closed this Jun 29, 2024

gerteck reopened this Jun 29, 2024

gerteck marked this pull request as ready for review June 29, 2024 08:35

yucheng11122017 reviewed Jun 30, 2024

View reviewed changes

gerteck added 2 commits July 1, 2024 10:40

Update tabgroup header prop name

c213c5c

Update tab group header prop

b1a0055

gerteck marked this pull request as draft July 3, 2024 07:23

gerteck added 3 commits July 4, 2024 10:14

Add slot for header back

a5c6934

The header named slot is still being used for navbar, hence removing it causes regression for dropdowns under navbars.

Rename prop name

1dd2449

To pass tests

Add Tabset, Tab, Tabgroup snapshot test

2168083

Merge branch 'master' into branch-TabGroupHeaderFix

0cdf3d0

gerteck marked this pull request as ready for review July 4, 2024 07:26

gerteck added 2 commits July 4, 2024 15:33

Refactor import

a5da4c4

Remove unnecessary import

Merge branch 'branch-TabGroupHeaderFix' of https://github.com/gerteck…

49ab2db

…/markbind into branch-TabGroupHeaderFix

Tim-Siu approved these changes Jul 6, 2024

View reviewed changes

Tim-Siu merged commit 54a60bf into MarkBind:master Jul 13, 2024
9 checks passed

Tim-Siu added c.Bug 🐛 p.Medium pr.BugFix 🐛 Fixes correct a programming error/assumption labels Jul 13, 2024

Tim-Siu added d.hard r.Minor Version resolver: increment by 0.1.0 labels Jul 13, 2024

tlylt added r.Patch Version resolver: increment by 0.0.1 and removed r.Minor Version resolver: increment by 0.1.0 labels Sep 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Tab-Group Header not displayed #2557

Fix Tab-Group Header not displayed #2557

gerteck commented Jun 14, 2024 •

edited

Loading

lhw-1 commented Jun 15, 2024

lhw-1 Jun 15, 2024 •

edited

Loading

gerteck commented Jun 20, 2024 •

edited

Loading

Tim-Siu commented Jun 22, 2024

codecov bot commented Jun 30, 2024 •

edited

Loading

yucheng11122017 left a comment

yucheng11122017 Jun 30, 2024

gerteck Jul 1, 2024

gerteck Jul 3, 2024 •

edited

Loading

Tim-Siu commented Jul 1, 2024

gerteck commented Jul 4, 2024 •

edited

Loading

Tim-Siu left a comment

github-actions bot commented Jul 13, 2024

tlylt commented Jul 13, 2024

Tim-Siu commented Jul 13, 2024

tlylt commented Jul 14, 2024

	<div
	v-else
	ref="dropdown"
	:class="[{ 'disabled': disabledBool }, 'btn-group', addClass]"
	>
	<slot name="before"></slot>
	<slot name="button">
	<button
	type="button"
	class="btn dropdown-toggle"
	:class="[btnType, btnWithBefore, { 'dropdown-toggle-split': hasBefore }]"
	:disabled="disabledBool"
	data-bs-reference="parent"
	data-bs-toggle="dropdown"
	>
	<slot name="header"></slot>
	</button>
	</slot>
	<slot name="dropdown-menu" :class="[{ 'show': show }, { 'dropdown-menu-end': menuAlignRight }]">
	<ul class="dropdown-menu" :class="[{ 'show': show }, { 'dropdown-menu-end': menuAlignRight }]">
	<slot></slot>
	</ul>
	</slot>
	</div>

Fix Tab-Group Header not displayed #2557

Fix Tab-Group Header not displayed #2557

Conversation

gerteck commented Jun 14, 2024 • edited Loading

lhw-1 commented Jun 15, 2024

lhw-1 Jun 15, 2024 • edited Loading

Choose a reason for hiding this comment

gerteck commented Jun 20, 2024 • edited Loading

Tim-Siu commented Jun 22, 2024

codecov bot commented Jun 30, 2024 • edited Loading

Codecov Report

yucheng11122017 left a comment

Choose a reason for hiding this comment

yucheng11122017 Jun 30, 2024

Choose a reason for hiding this comment

gerteck Jul 1, 2024

Choose a reason for hiding this comment

gerteck Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Tim-Siu commented Jul 1, 2024

gerteck commented Jul 4, 2024 • edited Loading

Tim-Siu left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 13, 2024

tlylt commented Jul 13, 2024

Tim-Siu commented Jul 13, 2024

tlylt commented Jul 14, 2024

gerteck commented Jun 14, 2024 •

edited

Loading

lhw-1 Jun 15, 2024 •

edited

Loading

gerteck commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 30, 2024 •

edited

Loading

gerteck Jul 3, 2024 •

edited

Loading

gerteck commented Jul 4, 2024 •

edited

Loading