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

navbar compatibility #4

Merged
merged 15 commits into from
Nov 22, 2019
Merged

navbar compatibility #4

merged 15 commits into from
Nov 22, 2019

Conversation

jcheng5
Copy link
Member

@jcheng5 jcheng5 commented Nov 7, 2019

The purpose of this PR is to make it possible for bs3 navigation bar markup to work properly when bs4 is loaded on the page (without interfering with bs4 navigation bars, if they happen to be on the page also).

Testing notes

There's a test app under test-apps/bs3-navs/app.R. You can either check out the repo and navigate there, or just download the two files at https://github.com/rstudio/bootsass/tree/master/test-apps/bs3-navs into the same directory and run the app.R.

  • Ensure that navigation works for all except the "RMarkdown Website" nav bar
  • Ensure that if a tab or pill is activated it looks like it is activated (tab comes to the front, pill is highlighted in blue)
    • If the selected item is on a sub-menu, then the sub-menu should look active, and the selected item should also look active
  • Try making the browser window narrow to put it in mobile mode, and make sure everything works
  • Disabled items should be disabled and not be clickable
  • Test different themes (e.g., change bootstrap() to bootstrap(theme = "cosmo")) and ensure that the "bs3 navbarPage" section looks similar to the "bs4 navbar - default" section. Also that the "bs3 navbarPage (inverse)" section looks similar to the "bs4 navbar (dark)" section

TODO

  • Clean up tests
  • I think some shim JS needs to be removed from the Shiny branch
  • Remove bs4 nav-related styles from Shiny branch
  • For bs3 navbar drop-downs, it's possible to click on the <li> but not the <a>, which causes a no-op
  • For bs3 navbar-inverse, drop-down menus have the wrong coloring (investigated and this problem exists in bs4 as well)
  • $(nav_anchor).tab("show") needs to work for both bs3 and bs4

@cpsievert
Copy link
Collaborator

Did you mean to check in a test app?

@jcheng5
Copy link
Member Author

jcheng5 commented Nov 9, 2019

There doesn't appear to be a dark-mode dropdown menu in bootstrap 4. This is what happens in straight bootstrap 4 if you change one of the examples from .navbar-light.bg-light to .navbar-dark.bg-dark:

image

And also looking at the _dropdown.scss file in bootstrap, there only appears to be one set of colors.

I'm not saying we shouldn't fix it, just that it may go into the category of "ways to make bootstrap 4 even better" rather than "ways to make bootstrap 3 work in bootstrap 4".

@jcheng5 jcheng5 marked this pull request as ready for review November 18, 2019 22:57
@jcheng5 jcheng5 force-pushed the joe/feature/navbar-compat branch from 5f09701 to 7642a23 Compare November 18, 2019 23:03
@jcheng5 jcheng5 requested a review from cpsievert November 18, 2019 23:05
@cpsievert
Copy link
Collaborator

Looking good thus far! Just FYI, we may want to consider a shim for .tabset-dropdown rstudio/rmarkdown#1405

@cpsievert
Copy link
Collaborator

cpsievert commented Nov 19, 2019

@jcheng5 consider 17a5f23 experimental at this point (just want to see if you think this is viable thing to do)

And now that I think of it, maybe this JS should live in rmarkdown and replace the existing .tabset-dropdown CSS? That CSS is already known to have issues, even with BS3

@cpsievert
Copy link
Collaborator

I think we should set justify-content: end for navbar-right?

Screen Shot 2019-11-19 at 10 49 07 AM

@cpsievert
Copy link
Collaborator

For posterity, BS3 style content fading doesn't appear to work currently, but I'll approve and file an issue so we can get moving in the rmarkdown pull request

library(bootscss)
library(htmltools)
browsable(withTags(body(
  bs4_sass(),
  ul(
    class="nav nav-tabs", role="tablist",
    li(
      role="presentation", class="active",
      a(
        href="#home", "aria-controls" = "home", role="tab", "data-toggle"="tab",
        "Home"
      )
    ),
    li(
      role="presentation",
      a(
        href="#about", "aria-controls" = "about", role="tab", "data-toggle"="tab",
        "About"
      )
    )
  ),
  div(
    class="tab-content",
    div(
      role="tabpanel", class="tab-pane fade in active", id="home",
      "Foo"
    ),
    div(role="tabpanel", class="tab-pane fade", id="about",
        "Bar"
    )
  )
)))

@jcheng5 jcheng5 merged commit 2175554 into master Nov 22, 2019
@jcheng5 jcheng5 deleted the joe/feature/navbar-compat branch November 22, 2019 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants