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

feat: Use margin-bottom/gap spacing in more places #725

Merged
merged 16 commits into from
Aug 7, 2023
Merged

Conversation

gadenbuie
Copy link
Member

Fixes #672

  • For consistency, I renamed .bslib-mb-spacer to .bslib-mb-spacing. I'm hoping this helps when trying to remember the class/variable names. The reasoning is that --bslib-spacer (the custom CSS property) is non-commital since it hasn't been applied anywhere. --bslib-mb-spacer is a similar unit. But once it's a class, we use -spacing, i.e. .bslib-gap-spacing and .bslib-mb-spacing, which I read as "the elements in this container use gap spacing or this element uses margin bottom spacing". Obviously, it's not perfect.

    • Alternative: we could use .bslib-mb or .mb-bslib instead, since there is a difference that this class directly sets the margin bottom of the current element.
  • While here I added something that feels relevant: setting margin-bottom: 0 on fill items with bslib's margin bottom spacing that are activated within a fillable container.

  • layout_columns() and layout_column_wrap() gain the .bslib-mb-spacing class so that they naturally have some bottom margin in a flow context. This also motivated adding the above consideration for .bslib-mb-spacing fill items.

  • sidebar() gains gap and padding arguments to control the gap and padding within the sidebar itself. gap picks up from --bslib-spacer or the inline style, and I added a CSS variables --bslib-sidebar-content-padding because --bslib-sidebar-padding needs to be a single value and is used in a few places that aren't specifically a padding property. The .sidebar-content container also gains the .bslib-gap-spacing so that bottom margin is dropped in sidebars in favor of the gap spacing.

@gadenbuie gadenbuie requested a review from cpsievert August 4, 2023 20:12
R/sidebar.R Outdated Show resolved Hide resolved
Copy link
Collaborator

@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.

Thanks, probably worth a NEWS item?

@gadenbuie
Copy link
Member Author

Thanks, probably worth a NEWS item?

I added a news item for sidebar(gap=, padding=), but we haven't really discussed the gap/margin-bottom features explicitly anywhere else. Should we consider adding a small article?

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.

sidebar() should have padding/gap args
2 participants