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

Awkward usage for layout_column_wrap #732

Open
wch opened this issue Sep 15, 2023 · 1 comment
Open

Awkward usage for layout_column_wrap #732

wch opened this issue Sep 15, 2023 · 1 comment

Comments

@wch
Copy link
Collaborator

wch commented Sep 15, 2023

The signature for layout_column_wrap is this:

def layout_column_wrap(
    width: Optional[CssUnit],
    *args: TagChild | TagAttrs,
    fixed_width: bool = False,
    heights_equal: Literal["all", "row"] = "all",
    fill: bool = True,
    fillable: bool = True,
    height: Optional[CssUnit] = None,
    height_mobile: Optional[CssUnit] = None,
    gap: Optional[CssUnit] = None,
    class_: Optional[str] = None,
    **kwargs: TagAttrValue,
) -> Tag:

This makes for kind of awkward usage. Because width comes before *args, it can never be named when the function is called (assuming you are passing in some *args, which you would always want to do). For example:

# These two don't work
layout_column_wrap(width="200px", "Hello", "world")
layout_column_wrap("Hello", "world", width="200px")

# This works
layout_column_wrap("200px", "Hello", "world")

On the other hand, height must always be named, and come after the *args. So if you want to specify both the width and height, you'd have to do this, which is awkward:

layout_column_wrap(1/2, "Hello", "world", height="300px")

Another awkward thing is that if you want to set width to None, you must call the function like this:

layout_column_wrap(None, "Hello", "world")
@gadenbuie
Copy link
Collaborator

Your timing on bringing this up is great. I was just looking at updating the CSS used by layout_column_wrap() in such a way that width would not be required. The idea would be to use an auto-fit grid layout set such that width defines the minimum space required to create a new column in a row and we otherwise allow grid to choose the number of rows and columns. (I created a placeholder PR with the core idea here, but won't get back to this until after conf.)

With this change, it would make a lot of sense to move width out of the first argument position on the R side too, which sounds like an improvement for both languages. My proposal for R is that width default to something like "200px", as an optional named argument, with all the same behavior if the user chooses to customize the argument.

This would be a breaking change on the R side, so now would be a great time for any additional feedback or collaboration around this function.

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

No branches or pull requests

2 participants