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

Width and Height configuration for children of Rows and Columns #855

Closed
swankjesse opened this issue Mar 16, 2023 · 5 comments · Fixed by #1279
Closed

Width and Height configuration for children of Rows and Columns #855

swankjesse opened this issue Mar 16, 2023 · 5 comments · Fixed by #1279
Assignees

Comments

@swankjesse
Copy link
Collaborator

Problem Statement

I’ve got a column where I’d like to configure the minimum size of a child element. Perhaps something like this:

  @Composable
  Column {
    ...
    Button(
      layoutModifier = LayoutModifier
        .horizontalAlignment(CrossAxisAlignment.Stretch)
        .minHeight(40),
      text = theButtonLabel,
      ...
    )
  }

For this occurrence I think I want to specify a minimum height, because if the row’s contents exceed that height (perhaps due to a long label text-wrapping), I’m happy with the element being taller than that size.

In another part of my layout I’d like to specify an element’s exact width=0 with grow(1.0) so that I can achieve 50/50 balance for adjacent elements:

  @Composable
  Row {
    Button(
      layoutModifier = LayoutModifier
        .horizontalAlignment(CrossAxisAlignment.Stretch)
        .width(0)
        .grow(1.0),
      text = "LEFT",
    )
    Button(
      layoutModifier = LayoutModifier
        .horizontalAlignment(CrossAxisAlignment.Stretch)
        .width(0)
        .grow(1.0),
      text = "RIGHT",
    )
  }

Without locking the width in at 0, the side-by-side elements only balance if their contents are the exact same width.

Syntax

I’d like to express these layout goals in the API. The above syntax is illustrative; I’m not at all attached to it. Borrowing terms from HTML or iOS or Android would be ideal.

Margin, Padding, Size

We have to decide if width and height includes padding or not. My preference is we follow the standard CSS Box Model and not include padding in width or height.

I’d also like for us to rename Padding to Margin, if that’s closer to its behavior. In particular, if an element doesn’t paint its background in the padding area we should probably rename it to margin.

@colinrtwhite
Copy link
Collaborator

This is relatively straightforward to implement - we could override a child's minimum width/height by overriding the return value of Measurable.minWidth/Measurable.minHeight. Likewise, we can override an item's width/height by overriding the return value of Measurable.measure. That said, do we want to provide this level of control in Redwood? I think I'm OK with it as we can always deprecate/remove these functions later if they cause issues. Also I think your proposed syntax is good.

As for renaming Padding, that makes sense to me - it does behave more like Margin at the moment. My only minor concern is margin is less discoverable (at least to me). Agreed that Margin shouldn't be counted as part of width/height.

Also semi-related, but I'm planning to introduce a Dp type (similar to Compose UI's Dp class) to give us better type safety. It'll enforce the value is positive in the guest code and include utils in the host code to convert Dp values into platform pixel values. So we would have:

Button(
  layoutModifier = LayoutModifier
    .horizontalAlignment(CrossAxisAlignment.Stretch)
    .width(0.dp)
    .grow(1.0),
  text = "RIGHT",
)

@swankjesse
Copy link
Collaborator Author

Regarding Dp, I like it. CSS has valid use cases for negative margins (explainer) so I don’t think that requiring non-negative values there is necessarily a win.

@veyndan
Copy link
Contributor

veyndan commented Jun 15, 2023

We have to decide if width and height includes padding or not. My preference is we follow the standard CSS Box Model and not include padding in width or height.

I think we should opt for the "alternative CSS box model", where the width and height does include the padding.

I've often seen * { box-sizing: border-box; } included in many (slightly-opinionated) CSS reset styles (1, 2). It didn't make it into the de-facto CSS reset style by Eric Meyer, but that is probably because box-sizing wasn't a consistently supported property in 2011.

@veyndan
Copy link
Contributor

veyndan commented Jun 15, 2023

As for renaming Padding, that makes sense to me - it does behave more like Margin at the moment.

@colinrtwhite Is this still the case? After adding some margin to the Row in Emoji Search, it looks like it's behaving like padding.

Screenshot 2023-06-15 at 15 11 34

@colinrtwhite
Copy link
Collaborator

@veyndan It behaves as padding when passed as an argument to the Row/Column functions, but it applies margin to any child items that have margin added using Modifier.margin(). This is because we can't add margin outside Row/Column since we don't control how it's drawn outside of its bounds. We would need to apply the margin using the native APIs, which could have subtle differences.

Margin applied to children inside of Row/Column is handled using the layout engine since we have full control over how things are laid out.

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 a pull request may close this issue.

3 participants