Component update - Column/Columns

viktoria-schwarz commented 3 years ago

Description

We use a 12 columns layout so far, but we should have one based on 8 as in https://confluence.rsvx.it/display/DESIGN/Layout with respective gutters and margins.

Context

Adjust our (b2b) components to what is specified.

Consequences

We have a Column and Columns component that works as described in the link above.

sebald commented 2 years ago

Why not allow both?!

viktoria-schwarz commented 2 years ago

I am fine with allowing both 8 and 12 column layout, but why wouldn't we stick to Toshi's specification if we do it in all the other concerns aswell..

sebald commented 2 years ago

We also should fix the padding "issue" 😄

viktoria-schwarz commented 2 years ago

I found out what is a bit off in the <Column> component: it doesn't stack its children vertically, but horizontally. In a column that I want to fill with content (aka. children elements), I expect them to appear below each other, same as in a line you expect your content next to each other - until you specify a break. In the <Column>, you can nevertheless have multiple children in a column cell by using a container component.

Thoughts?

sebald commented 2 years ago

I found out what is a bit off in the <Column> component: it doesn't stack its children vertically, but horizontally. In a column that I want to fill with content (aka. children elements), I expect them to appear below each other, same as in a line you expect your content next to each other - until you specify a break. In the <Column>, you can nevertheless have multiple children in a column cell by using a container component.

Thoughts?

Can you clarify, maybe with an illustration? I am not sure I can follow 🤔

viktoria-schwarz commented 2 years ago

Okay. What our Columns/Column components do right now is the following:

// example with 2 columns, hence width=6 
<Columns>
    <Column width={6}>
        <Badge>Children 1</Badge>
        <Badge>Children 2</Badge>
        <Badge>Children 3</Badge>
    </Column>
    <Column width={6}>
        <Badge>Children 1</Badge>
        <Badge>Children 2</Badge>
        <Badge>Children 3</Badge>
    </Column>
</Columns>

This renders as 2 columns with 1 row each.

I would expect that this renders as 2 columns with 3 rows each. (The column is stacking its children horizontally, not vertically, as I described before).

If we change the way our <Column> component stacks its children, the question remains: what if I WANT to have multiple children next to each other and not below each other? Well, use a <Container>.

sebald commented 2 years ago

Okay, I would not expect this actually. If I see this, I expect two 50% columns and the content grows vertically. Or whatever for that matter, since the layout doesn't care what "happens" inside a <Column>. Can be inline or blocks, etc.

I guess you expect something more like the bootstrap columns? https://getbootstrap.com/docs/5.0/layout/columns/

If we change the way our component stacks its children, the question remains: what if I WANT to have multiple children next to each other and not below each other? Well, use a .

I guess what you want to do is something like inline, grid or tiles? Like braid does: https://seek-oss.github.io/braid-design-system/foundations/layout

viktoria-schwarz commented 2 years ago

I expect two 50% columns and the content grows vertically.

Isn't that exactly what I meant? And I think in this case, the layout should matter what happens inside a <Column>, since a "column" in its natural sense implies a vertical stacking of its elements, just like a column in Excel or so. Sure, it's similar to a grid, and I would prefer having a grid (like it is actually wanted by Toshi), but for me if we leave our <Column> as it is, the only benefit of using it would be that I can specify its width on a 12-scale? I don't see a point in having a <Column> component if you would need another layout component (Stack) to have your elements one below the other... this makes it unnecessarily complicated.

sebald commented 2 years ago

Isn't that exactly what I meant?

Not quite, because I don't expect the elements (badge) in your case to each be in a "new row". I said "grow" because I expect that if inline elements need more vertical space than given (50%) the column will grow vertically.

I think I get what you want and I think you misinterpret it 🤔 The child elements of a <Column> van not be "stacked". Otherwise you can not create a whole set of layouts.

e.g.

sebald commented 2 years ago

I think the important part, what you maybe miss in your interpretation is, that the contents of a column can (and is allowed to) be different in size (width).

viktoria-schwarz commented 2 years ago

Yes they can be of different width, but then you can still use a Container/Box and arrange it inside of it?

sebald commented 2 years ago

Yes they can be of different width, but then you can still use a Container/Box and arrange it inside of it?

Yes, because the Box is the most low level component we have. You can also reimplementiert that with dibs and useStyles.

But we want to make it as convenient as possible for the users. So having to do the least amount of work is preferable.

Not sure what your argument is 🤔

viktoria-schwarz commented 2 years ago

I'm just thinking of usecases for a column/columns. For me, this would be when you want to stack different elements (one per row) next to each other. And then it's the most convenient to have a children of <Column> vertically stacked. If you want to have multiple elements arranged in a row, use <Container> or <Box>. My argument is just that I am convinced that a user's intuition and expectation is that the children elements of a <Column> are stacked vertically and not horizontally. (Also in other grid-like frameworks, like Excel etc., where you work with rows and columns, after entering content to a column, it jumps vertically within that column and not horizontally to the next one.)

sebald commented 2 years ago

Yes, but like I said, if this is the behavior you change the default of how HTML/CSS flows and we might exclude a whole set of layouts this way.

I can not really follow your example, can you make post a picture of a layout? 😄

An also: You said it your self. Excel is a grid not columns and rows.

viktoria-schwarz commented 2 years ago

I think we don't have so different opinions in the end, just maybe different use cases in mind, but I don't mind if you say it's a default as it is now.

What I meant with the Excel comparison is more that when you enter a value in a certain column, hit enter, then it jumps to the next row (vertically! in the same column). But then we can leave the Column/Columns components, or do we still need to fix the padding issue?

ti10le commented 2 years ago

I don't know if I can follow this thread. But I think we need a decision to make the Column component a little bit better. I think I have the same understanding like Sebastian.

ti10le commented 2 years ago

This image means for me: `

... ... ` In my opinion the Column don't have to know whats inside him. If we stack his children than we can't do something like in the second Column of this image. What do you mean how we go further with this issue @sebald @viktoria-schwarz? What are the todos?

viktoria-schwarz commented 2 years ago

I agree that the Column component doesn't need to know what the children are, it's just about whether the children are stacked vertically or horizontally - but if both of you say it should be horizontally, we can do it like that. In my understanding, the Column/Columns components already work like this and so we can leave it as is, except for the padding issue (we have a 12 columns layout right now). Do we need to rebuild the component so that one can specify the max. columns themself?

ti10le commented 2 years ago

We would need to modify the Column component to accept a maxWidth. But these are only adjustments to the properties.

ti10le commented 2 years ago

Final Todos:

[x] add switch case block for horizontal and vertical alignment and show error message if theres no matching prop type
[x] make more expressive examples in marigold docs
[x] make sure that horizontalAlign and verticalAlign are working correctly

ti10le commented 2 years ago

@sebald Would you rather use a 8 or 12 Grid or both and make a var to choose one of them?

now it's 12 like the most systems
toshis layout says 8

sebald commented 2 years ago

@sebald Would you rather use a 8 or 12 Grid or both and make a var to choose one of them?

now it's 12 like the most systems

toshis layout says 8

Let's stick with the 12 for now, until we have an actual use case.

marigold-ui / marigold