Skip to content

Conversation

@JohnCoene
Copy link
Contributor

This pull request removes the bootstrap dependency. Bootstrap would often clash when used with rmarkdown, blogdown, or shiny applications that do not import Bootstrap or use a different version than that which is used by crosstalk.

Bootstrap seems to only be necessary for some minor styling (e.g.: DT with style="bootstrap") and the bscols function (bootstrap grid). Removing bootstrap will "break" the former but using flexbox the responsive grid from bootstrap can be closely mimicked.

This thus essentially mainly changes CSS, adding a crosstalk-row class, as well as crosstalk-column-* (1 through 12) classes. This, however, deprecates the device argument of the bscols.

@CLAassistant
Copy link

CLAassistant commented May 13, 2021

CLA assistant check
All committers have signed the CLA.

@cpsievert
Copy link
Contributor

Thanks @JohnCoene. I don't think we'll go in this direction since it's a pretty big breaking change, but we'll likely be providing a way to opt-out of including Bootstrap in a future release

@JohnCoene JohnCoene closed this May 13, 2021
@cpsievert

This comment has been minimized.

@cpsievert cpsievert reopened this May 20, 2021
@cpsievert
Copy link
Contributor

@JohnCoene after a bit more internal discussion, I think we will go in this direction (removing Bootstrap by default). That said, we should probably keep most of the HTML/CSS classes the same so that, if Bootstrap happens to be relevant, the behavior doesn't change.

Also, instead of re-implementing Bootstrap grid to support bscols(), it seems better to precompile Bootstrap's grid CSS and attach that as a dependency to bscols():

library(bslib)
bs3 <- bs_theme(version = 3)
exclude <- setdiff(unique(names(bs3$layers)), c("_grid", ""))
sass::sass(bs_remove(bs3, exclude))

Copy link
Contributor

@cpsievert cpsievert left a comment

Choose a reason for hiding this comment

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

See comments and also we should look into whether we should bother adding CSS to support the form-group and control-label class on filter_select()/filter_slider().

Also, feel free to reach out to me carson@rstudio.com if you want to discuss any of this in more depth

@cpsievert
Copy link
Contributor

Thanks @JohnCoene, this is looking pretty good overall. I'll get you some more feedback tomorrow after I discuss with @jcheng5

@JohnCoene
Copy link
Contributor Author

JohnCoene commented Jun 2, 2021

@cpsievert thank you but there is still a lot I need to do and test, I only just reverted what I did for the columns.

I'm not sure it's worth looking at just yet. I'll have a quick call with Alison next week to talk a bit about this.

I love your idea of taking the grid from Bootstrap though it does not seem to be fully working on my end. I'm going to look into that and see if I can come up with a solution. I'll ask you for pointers if I cannot figure it out.

@JohnCoene
Copy link
Contributor Author

Progress of a kind! Including the scaffolding layer fixes the grid system.

exclude <- setdiff(
  unique(names(bs3$layers)), 
  c(
    "_grid", 
    "", 
    "_scaffolding"
  )
)

@cpsievert cpsievert added this to the Next release milestone Jun 10, 2021
@JohnCoene
Copy link
Contributor Author

@cpsievert I think this should be good now. Please let me know anything that needs changing.

@JohnCoene
Copy link
Contributor Author

@cpsievert One thought, not sure it's very relevant. To make the bootstrap grid work I had to import _scaffolding.scss which includes the .container* classes. Is that an issue?

I think it's a common class name and that other frameworks may use it; I'm not sure how likely it is to cause clashes. Would it be an issue?

@cpsievert
Copy link
Contributor

cpsievert commented Jun 15, 2021

One thought, not sure it's very relevant. To make the bootstrap grid work I had to import _scaffolding.scss which includes the .container* classes. Is that an issue? I think it's a common class name and that other frameworks may use it; I'm not sure how likely it is to cause clashes. Would it be an issue?

Oh, thanks for bringing this up. Including all scaffolding is likely going to be problematic, but grid wasn't working becuase it assumes box-sizing: border-box. I'll fix this and take it over the finish line in #101

@cpsievert cpsievert closed this Jun 15, 2021
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.

3 participants