Rework tabset-dropdown to use regular Bootstrap dropdowns #2054

sdiebolt · 2021-02-24T09:27:16Z

This PR reworks the .tabset-dropdown class to work similarly to regular tabsets. It should also close #1697.

I wasn't satisfied with the current .tabset-dropdown class and wanted to use regular Boostrap dropdowns included in navs. This lead me to remove the .tabset-dropdown CSS and improve the buildTabsets JS function to include the .tabset-dropdown class.

The .tabset-dropdown class can now be added to any sub-header of a header with class .tabset or .tabset-pills. The tabset-dropdown header will determine the dropdown's name and all of its sub-headers will be added as dropdown tabs.

Example

---
title: "RMarkdown Dropdown"
output: 
  html_document:
    self_contained: false
    theme:
      version: 4
---

# First header

Lorem Ipsum

# Dropdown {.tabset .tabset-fade}

## Dropdown {.tabset-dropdown}

### Choice 1

Dropdown 1

### Choice 2

Dropdown 2

### Choice 3

Dropdown 3

## Tab 1

Tab 1

## Tab 2

Tab 2

## Tab 3

Tab 3

# Second header

Lorem Ipsum

Headers using the tabset-dropdown class now work similarly to regular tabsets: all subheaders are included in a bootstrap dropdown. This dropdown will be included inside the tabset just above.

CLAassistant · 2021-02-24T09:27:20Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

yihui

I love this idea (and I'm glad that it could fix #1697), but I didn't review the implementation carefully. I'll let @cderv review and test it later. Thanks!

cderv · 2021-06-01T12:36:46Z

For reference while reviewing this PR, bslib is in the process of coming up with new UI function for creating navs & tabs : rstudio/bslib#296
It is partly already there: https://rstudio.github.io/bslib/reference/index.html#section-create-navs-and-navbars

I believe this will be possible to use it in R Markdown document using Bootstrap also.

sdiebolt · 2021-06-01T13:20:54Z

Thanks for pointing out the new features from bslib, they will help me a lot!

Please note that I am not a regular JS user: feel free to be as critical with my implementation as needed.

cderv

Thanks for this PR.

I made very few comments on the code. Overall, it seems to work fine.
I test with bslib and without.

What I am not sure about is what the previous behavior was supposed to be.
I tested your example without the PR (using dev rmarkdown) and I got a very different result - it puzzles me because I did not know about this hidden feature of rmarkdown 😅
But being different, this would mean than any user already using .tabset-dropdown would have its document broke with this PR probably. However, the result of this PR seems closer to Bootstrap dropdown's in tabsets.

For reference, the previous design looked somewhat like this #1405 (comment)
The aim was to fix #1116 by offering a dropdown feature in tabs. So if the design is different, the same feature can be achieved using this new interface. @yihui you seemed ok for the change, did you thought of this ?

Also, we are now in a mix of Boostrap 3 and Bootstrap 4 support, with different version : brought by rmarkdown itself or by bslib. That is a lot of things to test and the HTML come written by the JS script is a mix of BS3 and BS4 syntax to me.

I asked @cpsievert to have a look and give its thoughts on this one. We need to think this through correctly on what it is supposed to be.

@yihui this tabsets feature is something we may consider doing using Lua filter maybe instead of using only JS to post process the HTML directly. Changing to a Fenced div syntax is also an option.

For the record, while reviewing this PR, I can now confirm that the tabset feature is only working currently with --section-divs activated for Pandoc with HTML 4 output. (section_divs = TRUE in html_document()).

cderv · 2021-06-01T11:10:26Z

inst/rmd/h/navigation-1.1/tabsets.js

+      }
+
+      // check if this is a dropdown tab and process it accordingly
+      var dropdown = tab.hasClass("tabset-dropdown");


This is comestic maybe but I would put this variable with the others defined above (l. 54) so that all variables are based on class are in the same place in code

cderv · 2021-06-01T12:55:29Z

inst/rmd/h/navigation-1.1/tabsets.js

+        li.addClass('dropdown');
+
+        // build and append the dropdown toggle
+        var a = $('<a class="dropdown-toggle" data-toggle="dropdown" href="#">'


Suggested change

var a = $('<a class="dropdown-toggle" data-toggle="dropdown" href="#">'

var a = $('<a class="dropdown-toggle" data-toggle="dropdown" href="#" role="button" aria-expanded="false">'

as in bootstrap doc - I think it is better for accessibility

cderv · 2021-06-01T13:15:17Z

inst/rmd/h/navigation-1.1/tabsets.js

+          var dropdownId = dropdownTab.attr('id');
+
+          // remove any table of contents entries associated with
+          // this ID (since we'll be removing the heading element)
+          $("div#" + tocID + " li a[href='#" + dropdownId + "']")
+            .parent().remove();
+
+          // sanitize the id for use with bootstrap tabs
+          dropdownId = dropdownId
+            .replace(/[.\/?&!#<>]/g, '')
+            .replace(/\s/g, '_');
+          dropdownTab.attr('id', dropdownId);
+
+          // get the heading element within it, grab it's text, then remove it
+          var dropdownHeading = dropdownTab
+            .find('h' + dropdownTabLevel + ':first');
+          var dropdownHeadingText = dropdownHeading.html();
+          dropdownHeading.remove();
+
+          // build and append the dropdown tab list item
+          var dropdownLi = $('<li role="presentation"></li>');
+          dropdownMenu.append(dropdownLi);


Could we make a JS function as this is the same process as for other tabs above but with different parameter ? Refactoring would help later maintaining

cderv · 2021-06-01T13:35:44Z

inst/rmd/h/navigation-1.1/tabsets.js

+        // build and append the tab link
+        var a = $('<a role="tab" data-toggle="tab">' + tabHeadingText + '</a>');


Suggested change

// build and append the tab link

var a = $('<a role="tab" data-toggle="tab">' + tabHeadingText + '</a>');

// build and append the tab link

var a = $('<a role="tab" data-toggle="tab" role="button" aria-expanded="false">' + tabHeadingText + '</a>');

Same as above

cpsievert · 2021-06-01T16:12:12Z

I asked @cpsievert to have a look and give its thoughts on this one. We need to think this through correctly on what it is supposed to be.

I would be more conservative here and not intentionally break the existing .tabset-dropdown functionality and instead add a new .dropdown (or similar) class to implement this new behavior.

On a larger note, if the plan is to require Bootstrap 4+ sometime in the near future, maybe it's time to use the same lua filters as quarto (or bslib via remarker) to implement tabsets. The bslib way should be able to support Bootstrap 3 out-of-the-box, but the quarto way would probably require Bootstrap 4+ (unless, at run-time, you have some way of knowing what version in relevant).

cderv · 2021-06-02T10:21:40Z

I would be more conservative here and not intentionally break the existing .tabset-dropdown functionality and instead add a new .dropdown (or similar) class to implement this new behavior.

We could indeed try to support both using a new class. 🤔 I am really wondering how many people use this feature that was never documented. It see a few though : https://github.com/search?q=.tabset-dropdown+extension%3ARmd&type=Code&ref=advsearch&l=&l=

On a larger note, if the plan is to require Bootstrap 4+ sometime in the near future,

Using bslib by default is a plan yes. Default to 4 for html_document() would be another topic - We are talking of being conservative for tabsets, so I guess we must be for Bootstrap version too. We are starting thinking about it and we'll track this in a new issue.

maybe it's time to use the same lua filters as quarto

The syntax for tabsets in Quarto is different (using fenced div) so this would break the current usage. And Quarto does not support all the option for tabset that are supported in R Markdown (likes pills or fade).
So it would require to adapt the filter to be conservative on feature and syntax.
But yes, as in my first comment, Lua filter is clearly a good way to implement such feature.

bslib via remarker. The bslib way should be able to support Bootstrap 3 out-of-the-box, but the quarto way would probably require Bootstrap 4+ (unless, at run-time, you have some way of knowing what version in relevant).

I'll ask Winston about this project to know more. Being able in some way to sure bslib directly without re-doing any bootstrap feature directly would be great.

Thanks for your feedback @cpsievert

yihui · 2021-06-03T14:46:42Z

@yihui this tabsets feature is something we may consider doing using Lua filter maybe instead of using only JS to post process the HTML directly. Changing to a Fenced div syntax is also an option.

@cderv I don't care about the implementation. Either JS or Lua is fine to me. I'm also okay with changing the syntax. This is an undocumented feature and doesn't seem to be widely used. If a breaking change can make it easier to maintain in the future (and more compatible with quarto), I'm okay with making a breaking change.

tabset-dropdown now uses Boostrap instead of CSS

4c14194

Headers using the tabset-dropdown class now work similarly to regular tabsets: all subheaders are included in a bootstrap dropdown. This dropdown will be included inside the tabset just above.

Removed left over code from active tab management

735051e

yihui approved these changes May 17, 2021

View reviewed changes

yihui requested a review from cderv May 17, 2021 22:24

yihui added the next to consider for next release label May 17, 2021

cderv self-assigned this May 18, 2021

cderv reviewed Jun 1, 2021

View reviewed changes

cderv mentioned this pull request Jun 2, 2021

support for tabs in BS4 r-lib/pkgdown#1694

Merged

cderv mentioned this pull request Oct 3, 2022

.tabset-dropdown's menu right icon doesn't render properly with self_contained: false #1697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework tabset-dropdown to use regular Bootstrap dropdowns #2054

Rework tabset-dropdown to use regular Bootstrap dropdowns #2054

sdiebolt commented Feb 24, 2021

CLAassistant commented Feb 24, 2021 •

edited

Loading

yihui left a comment

cderv commented Jun 1, 2021

sdiebolt commented Jun 1, 2021

cderv left a comment •

edited

Loading

cderv Jun 1, 2021

cderv Jun 1, 2021

cderv Jun 1, 2021

cderv Jun 1, 2021

cpsievert commented Jun 1, 2021 •

edited

Loading

cderv commented Jun 2, 2021

yihui commented Jun 3, 2021

	var a = $('<a class="dropdown-toggle" data-toggle="dropdown" href="#">'
	var a = $('<a class="dropdown-toggle" data-toggle="dropdown" href="#" role="button" aria-expanded="false">'

		// build and append the tab link
		var a = $('<a role="tab" data-toggle="tab">' + tabHeadingText + '</a>');

Rework tabset-dropdown to use regular Bootstrap dropdowns #2054

Are you sure you want to change the base?

Rework tabset-dropdown to use regular Bootstrap dropdowns #2054

Conversation

sdiebolt commented Feb 24, 2021

Example

CLAassistant commented Feb 24, 2021 • edited Loading

yihui left a comment

Choose a reason for hiding this comment

cderv commented Jun 1, 2021

sdiebolt commented Jun 1, 2021

cderv left a comment • edited Loading

Choose a reason for hiding this comment

cderv Jun 1, 2021

Choose a reason for hiding this comment

cderv Jun 1, 2021

Choose a reason for hiding this comment

cderv Jun 1, 2021

Choose a reason for hiding this comment

cderv Jun 1, 2021

Choose a reason for hiding this comment

cpsievert commented Jun 1, 2021 • edited Loading

cderv commented Jun 2, 2021

yihui commented Jun 3, 2021

CLAassistant commented Feb 24, 2021 •

edited

Loading

cderv left a comment •

edited

Loading

cpsievert commented Jun 1, 2021 •

edited

Loading