-
Notifications
You must be signed in to change notification settings - Fork 56
Remove bootstrap #97
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
Remove bootstrap #97
Conversation
|
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 |
This comment has been minimized.
This comment has been minimized.
|
@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 library(bslib)
bs3 <- bs_theme(version = 3)
exclude <- setdiff(unique(names(bs3$layers)), c("_grid", ""))
sass::sass(bs_remove(bs3, exclude)) |
There was a problem hiding this 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
|
Thanks @JohnCoene, this is looking pretty good overall. I'll get you some more feedback tomorrow after I discuss with @jcheng5 |
|
@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. |
|
Progress of a kind! Including the exclude <- setdiff(
unique(names(bs3$layers)),
c(
"_grid",
"",
"_scaffolding"
)
) |
|
@cpsievert I think this should be good now. Please let me know anything that needs changing. |
|
@cpsievert One thought, not sure it's very relevant. To make the bootstrap grid work I had to import 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 |
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 thebscolsfunction (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-rowclass, as well ascrosstalk-column-*(1 through 12) classes. This, however, deprecates thedeviceargument of thebscols.