Skip to content

Conversation

@gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Sep 14, 2023

Avoids items overflowing the parent layout container. With this rule, width becomes the point at which a new column is created, i.e. if width = "400px" then new columns are added to a row if there is at least 400px of space.

Breaking: This PR also makes width an optional argument and moves it to after the .... A code search shows that most people name the width argument, but in case they haven't we'll test to see if the first argument to layout_column_wrap() looks like a valid width value (a single numeric value or a valid CSS unit) and we'll emit a deprecation warning and use that first value as width.

Note: in the deprecation warning I'm assuming this breaking change will cause our next version to be v0.6.0 (in any case, we should review our deprecation warnings before the next release to make sure we have the right version in them).

Some code to try out this feature:

x <- lapply(1:3, \(x) card(lorem::ipsum(x)))

layout_column_wrap(!!!x)

# Explicit, named `width`
layout_column_wrap(width = 1/2, !!!x)
layout_column_wrap(width = "300px", !!!x)
layout_column_wrap(width = 400, !!!x)

# Implied as the first argument,
# leads to deprecation warning but gives same result
layout_column_wrap(1/2, !!!x)
layout_column_wrap("300px", !!!x)
layout_column_wrap(400, !!!x)

@gadenbuie gadenbuie marked this pull request as ready for review October 11, 2023 19:54
@gadenbuie gadenbuie self-assigned this Oct 11, 2023
Co-authored-by: Carson Sievert <[email protected]>
@cpsievert
Copy link
Collaborator

cpsievert commented Oct 18, 2023

I'm on board with these changes, but I'd like to see this PR split into 2: one for the min(%s, 100%) change (with a clear examples/explanation of where it makes a difference, as well as a dedicated bullet for it in the NEWS) and then another PR for the breaking change.

Also, feel free to use your own examples, but here is one that's fairly simple, but demonstrates the change in behavior for the 1st change:

# Open me on a medium sized screen (i.e., between about 800-500px) 
# and notice how the cards "bleed outside" of their containing column

my_card <- card(lorem::ipsum(2), max_height = 100)

page_fixed(
  layout_column_wrap(
    width = "800px", 
    class = "border border-danger",
    !!!replicate(3, my_card, simplify = F)
  )
)

@gadenbuie
Copy link
Member Author

@cpsievert I spun out the initial, non-breaking change into #851 and I'll rebase and re-word this PR after we merge that PR.

@gadenbuie gadenbuie marked this pull request as draft October 19, 2023 15:22
@gadenbuie gadenbuie changed the title feat(layout_column_wrap): Allow items to be smaller than min width breaking(layout_column_wrap): Make width an optional argument Oct 19, 2023
@gadenbuie
Copy link
Member Author

Closing this PR in favor of a smaller, more targeted PR.

@gadenbuie gadenbuie closed this Oct 19, 2023
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