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

Implement dropdown in tabsets #1405

Merged
merged 6 commits into from
Jul 18, 2018
Merged

Implement dropdown in tabsets #1405

merged 6 commits into from
Jul 18, 2018

Conversation

rich-iannone
Copy link
Member

This allows for a dropdown menu to take the place of a standard tabset. It is used by adding the .tabset-dropdown class to a header (# Heading {.tabset .tabset-dropdown}).

This fixes #1116 and incorporates the code provided by @stefanfritsch into default.html. This implementation, however, doesn't require an HTML include. An example .Rmd is provided here:

---
title: "tabset dropdown"
output:
  html_document
---

```{r setup, include=FALSE}
knitr::opts_chunk$set(echo = TRUE)
```

# R Markdown {.tabset .tabset-dropdown}

## Tab A

This is an R Markdown document. 

## Tab B

When you click the **Knit** button a document will be generated.

## Tab C

Here is a summary table.

```{r cars}
summary(cars)
```

## Tab D

Here is a plot:

```{r pressure, echo=FALSE}
plot(pressure)
```

# Here is another section {.tabset}

## Tab A

This is content in a standard tabset.

## Tab B

This is more content in a standard tabset.

@rich-iannone rich-iannone requested a review from yihui July 16, 2018 21:47
Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel our default HTML template is already bloated enough, so I'd like this feature to be enabled conditionally. You may consider turning the new CSS/JS code into an HTML dependency, and add the dependency dynamically if the class .tabset-dropdown is found in the HTML code (the search for the class does not have to be rigorous).

You may see how icon dependencies are added conditionally and dynamically for example:

rmarkdown/R/html_document.R

Lines 367 to 370 in e9db868

# navbar icon dependencies
iconDeps <- navbar_icon_dependencies(navbar)
if (length(iconDeps) > 0)
knitr::knit_meta_add(list(iconDeps))

@yihui yihui added this to the v1.11 milestone Jul 17, 2018
@yihui
Copy link
Member

yihui commented Jul 17, 2018

And we should add @stefanfritsch as a contributor (ctb) in DESCRIPTION if the code was borrowed from #1116. Ideally, we need him to sign the contributor agreement since the code is not trivial, otherwise we'd better implement this independently by ourselves.

@stefanfritsch
Copy link

I'd be happy to sign the contributor agreement

@jjallaire
Copy link
Member

@yihui I agree that the default html template is bloated, but this doesn't add a huge amount of new content (maybe 1KB?) so I don't think it warrants the effort to make it conditional (font awesome was much more heavyweight). i.e. Users who care about payload size won't be convinced they should use the format b/c documents are 1KB leaner.

@yihui
Copy link
Member

yihui commented Jul 17, 2018

@jjallaire I'm suggesting a separate dependency because 1) this PR adds 60 lines of code to our HTML default template that currently has 500 lines of code (i.e. increasing the number of lines by 10% for a corner feature); 2) I really hope we could factor out some of the CSS/JS code into separate HTML dependencies, too, and this could be a start. My point of view was completely from the package maintenance side (managing 10 orthogonal modules, each of 50 lines, is easier than managing a 500-line library in my eyes).

That said, I'm not totally against adding this directly and unconditionally to the template. We could factor out CSS/JS in the future when we have a chance (we probably never will, given that this is not really a critical issue).

@yihui
Copy link
Member

yihui commented Jul 17, 2018

@jjallaire
Copy link
Member

@yihui I agree that this could be a good start, but I also think that the "real" solution might be as you said to refactor everything to make it more lean by default. I'd be in favor of creating an issue for this and then pursuing it soon rather than externalizing just this code ad-hoc.

@yihui
Copy link
Member

yihui commented Jul 17, 2018

Sounds good. Let's merge this PR when we get the signed agreement from @stefanfritsch.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rich-iannone Could you replace tabs with two spaces, and consistently indent with two spaces? We also need a NEWS item. Thanks!

@jjallaire
Copy link
Member

Agreement received from @stefanfritsch

@rich-iannone
Copy link
Member Author

I can definitely reformat the CSS so that it fits in with the rest. One question: should I deactivate the RStudio setting Insert spaces for tab to get true space characters?

@yihui
Copy link
Member

yihui commented Jul 17, 2018

No. You should leave it checked. But this setting is only effective when you type and hit the Tab key in the editor. It does not automatically convert tabs to spaces when you copy and paste.

@rich-iannone
Copy link
Member Author

Okay, turns out I have spaces but RStudio, when moving over the spaces with arrow keys, gives the impression of tabs. Will make the changes now.

@yihui
Copy link
Member

yihui commented Jul 17, 2018

I wouldn't notice it, either, had I not looked at the diff page here on Github (tabs look wider than four spaces). In RStudio, a tab does look like four spaces visually (two double spaces).

@rich-iannone
Copy link
Member Author

@yihui could you have a quick look at the new NEWS item and see that it fits stylistically?

@yihui
Copy link
Member

yihui commented Jul 17, 2018

Looks good. I tested it and noticed a vertical scrollbar:

image

I wonder if that is expected. I hope to get rid of it if it is not too difficult.

@rich-iannone
Copy link
Member Author

Let me try to get rid of that scrollbar.

@rich-iannone
Copy link
Member Author

I think the hidden state for overflow-y in the tabset dropdown might be adequate here. At least in my browser tests it appeared fine.

@yihui
Copy link
Member

yihui commented Jul 17, 2018

Yes, it is easy to suppress a scrollbar, but I hope we can figure out the root reason why the scrollbar is there in the first place. If you just forcibly hide it, you may end up (partially) hiding content, too. I wish overflow-y: hidden could be the last resort.

@rich-iannone
Copy link
Member Author

I agree. Let me do some further research/experimentation on this.

@yihui
Copy link
Member

yihui commented Jul 17, 2018

This article might be helpful: https://css-tricks.com/findingfixing-unintended-body-overflow/ I have used its JS code before to fix a horizontal scrollbar, and I'm not sure if the same thing can be done for vertical overflow.

@rich-iannone
Copy link
Member Author

Thanks! This helps.

@yihui
Copy link
Member

yihui commented Jul 18, 2018

Using inline-table does make the scrollbar disappear, although I don't fully understand why (this is beyond my expertise now). There will be a minor issue only in Firefox, though:

image

Note that the bottom border of the tab title is missing. I'm fine with this. It is quite trivial compared to the scrollbar issue.

@rich-iannone
Copy link
Member Author

I've tried to address the Firefox issue but it's not as simple as removing the white background (that element also inherits another #fff background). It appears that the background in Firefox is drawn one pixel lower.

Given we could accept this, should I this merge now?

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's ship it and ignore the Firefox issue.

@rich-iannone rich-iannone merged commit 6c2eb96 into rstudio:master Jul 18, 2018
$$('.tabset-dropdown > .nav-tabs > li').click(function () {
$$(this).parent().toggleClass('nav-tabs-open')
});
});
Copy link
Member

@jcheng5 jcheng5 Jul 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This (295-299) can be improved using jQuery event delegation, it's a good habit to get into:

https://learn.jquery.com/events/event-delegation/

What I'm suggesting is $(document).on("click", ".tabset-dropdown > .nav-tabs > li", ...)

yihui pushed a commit to RLesur/rmarkdown that referenced this pull request Apr 1, 2019
* Include JS/CSS to enable dropdown tabsets

* Reformat some JS/CSS lines

* Update NEWS

* Set tabset-dropdown `overflow-y` to `hidden`

* Change `overflow-y` & `display` values
@apreshill apreshill mentioned this pull request Jul 3, 2019
13 tasks
@yihui yihui mentioned this pull request Nov 5, 2019
1 task
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Dropdown Options in Tabsets
5 participants