rstudio / bslib

Tools for theming Shiny and R Markdown via Bootstrap 3, 4, or 5.
https://rstudio.github.io/bslib/
Other
443 stars 50 forks source link

refactor(preset-shiny): Style updates and nudges #998

Closed gadenbuie closed 4 months ago

gadenbuie commented 4 months ago

I'll fill out this description with screenshots Monday, in the mean time, here's a tl;dr. These mostly affect only the preset="shiny" theme:

  1. Use a smaller, tighter $box-shadow-sm
  2. Apply $box-shadow-sm to popovers (previously unshadowed)
  3. Use this shadow as the default card shadow
  4. I added $bslib-spacer that defaults to $spacer, so that we can change --bslib-spacer without affecting Bootstrap spacing.
  5. Then I bumped $bslib-spacer for the shiny preset to 1.5rem. This lets most things breathe a bit more and ensures that spacing between elements in a sidebar layout match the padding in the main and sidebar areas. This did require separating one or two things from --bslib-spacer that were using it as a stand-in for 1rem.
  6. The title of page_sidebar() now matches page_navbar() exactly by using similar markup and .navbar classes to wrap the title element.

Sidebar Page

image

Navbar Page

image

cpsievert commented 4 months ago

Then I bumped $bslib-spacer for the shiny preset to 1.5rem.

I'm slightly concerned that this decision will look weird/inconsistent in practice since a somewhat arbitrary set of things will have 1rem vs 1.5rem spacing.

Is the motivation for increasing mainly just for spacing in the main content area of a page_sidebar() with cards? Could we restrict the increased padding to this context?

gadenbuie commented 4 months ago

I'm slightly concerned that this decision will look weird/inconsistent in practice since a somewhat arbitrary set of things will have 1rem vs 1.5rem spacing.

Agreed, which is why I mentioned that --bslib-gap-spacer would be a better name, since that's basically how we're using the spacer variable in bslib. Internally, after this PR, we only use it for gap, padding, and margin in places where the spacing is consistent. I do think there is an internal consistency to how we're using $bslib-spacer where it covers "external" padding/gap/margin. We should stick with $spacer for the "internal" spacing.

There is one place I should check, which is in the --bslib-spacer usage in the shiny preset datatables styles.

Is the motivation for increasing mainly just for spacing in the main content area of a page_sidebar() with cards? Could we restrict the increased padding to this context?

Yes that but also in layout_columns() and layout_column_wrap() and page_navbar(), etc. If we increased this value but only in the context of page_sidebar(), then we would be in a weird place where certain layouts/elements have more spacing in certain places and not others.

gregswinehart commented 4 months ago

Navbar title looks great! I just noticed one little discrepancy in the height between the two (I prefer the taller one.) But this shouldn't block its release at all, I can file an issue later. navbar height

gadenbuie commented 4 months ago

just noticed one little discrepancy in the height between the two (I prefer the taller one.) But this shouldn't block its release at all, I can file an issue later.

I looked into this enough to determine that it's the fault of the .nav-underline class (used in page_navbar()) and that it's exactly 1px shorter in height.

gadenbuie commented 4 months ago

@gregswinehart and I talked and decided to use the lower-key shadows and take away borders on top-level cards and value boxes, adding them back for nested cards/value boxes or under the .bslib-card-box-shadow-none class.

image