Skip to content

Grid alignment ergonomics pitfall #19272

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

Open
benfrankel opened this issue May 18, 2025 · 2 comments
Open

Grid alignment ergonomics pitfall #19272

benfrankel opened this issue May 18, 2025 · 2 comments
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible S-Needs-Design This issue requires design work to think about how it would best be accomplished

Comments

@benfrankel
Copy link
Contributor

benfrankel commented May 18, 2025

I want to spawn a grid with two columns, one right-aligned and one left-aligned. This is a very common use case, e.g. for a credits screen or settings menu. The recommended approach in CSS is to use selectors and style cascading to overwrite only the justify-self property on the appropriate grid items:

.grid > :nth-child(2) {
    justify-self: end;
}

However, this does not translate to Bevy, where properties live in a monolithic Node component with no support for selectors or style cascading. You have to set the properties manually on spawn instead:

fn grid() -> impl Bundle {
    (
        Node { display: Display::Grid, ..default() },
        children![
            (Node { justify-self: JustifySelf::End, ..default() }, ...),
            (Node { justify-self: JustifySelf::Start, ..default() }, ...),
            (Node { justify-self: JustifySelf::End, ..default() }, ...),
            (Node { justify-self: JustifySelf::Start, ..default() }, ...),
        ],
    )
}

Unfortunately, this approach destroys reusability, because there's no way to do something like this:

fn grid() -> impl Bundle {
    (
        Node { display: Display::Grid, ..default() },
        children![
            widget::label(...).with_justify_self(JustifySelf::End),
            widget::button(...).with_justify_self(JustifySelf::Start),
            widget::container(...).with_justify_self(JustifySelf::End),
            widget::color_picker(...).with_justify_self(JustifySelf::Start),
        ],
    )
}

And even if you could, this type of code is a major pain to tweak (e.g. add a column, change a column's alignment, rearrange grid items, etc.). Plus, if you make changes like spawning or despawning grid items after the initial spawn, the column alignment will break.

To fix these issues, you could define a custom GridAlignment { rows: Vec<AlignSelf>, columns: Vec<JustifySelf> } component and a system to keep grid items in sync.

However, this leads to another problem: Between template tracks and explicit grid placement, how can you determine which rows/columns a grid item spans? Luckily this is already solved internally by taffy, and the computed information is provided -- but Bevy doesn't expose this yet.

@greeble-dev greeble-dev added C-Feature A new feature, making something new possible A-UI Graphical user interfaces, styles, layouts, and widgets S-Needs-Design This issue requires design work to think about how it would best be accomplished labels May 19, 2025
@ChristopherBiscardi
Copy link
Contributor

I think this needs a better use case to base any changes on.

The Credits example here would actually be a description list in css (a list of terms and descriptions) which helps point to the idea that its actually a two-element widget for each row, not a single list of unrelated text as currently implemented. So you don't need the enumerate, etc and the implementation makes sense as paired output for each row.

fn grid(content: Vec<[&'static str; 2]>) -> impl Bundle {
    (
        Name::new("Grid"),
        Node {
            display: Display::Grid,
            row_gap: Px(10.0),
            column_gap: Px(30.0),
            grid_template_columns: RepeatedGridTrack::px(2, 400.0),
            ..default()
        },
        Children::spawn(SpawnIter(content.into_iter().flat_map(|content| {
            [
                (
                    widget::label(content[0]),
                    Node {
                        justify_self: JustifySelf::End,
                        ..default()
                    },
                ),
                (
                    widget::label(content[1]),
                    Node {
                        justify_self: JustifySelf::Start,
                        ..default()
                    },
                ),
            ]
        }))),
    )
}

Unfortunately, this approach destroys reusability, because there's no way to do something like this:
..
widget::label(...).with_justify_self(JustifySelf::End),

From a widget/component lib design perspective: If a Widget's behavior is supposed to be modifiable by an end user, that needs to be part of its API. It largely shouldn't be override-able from outside as there's no way to tell which interior node the styles should apply to. You could say "always the root element" but then what happens to fragments, arrays, etc.

The argument is made sometimes (in web circles especially) that any widget should allow any base-level style modification, but this means you lose control over the widget's behavior and can't evolve the widget's API without fixing all usage sites (which aren't particularly discoverable as there is no actual compiler-level break, its all visual breakage at runtime). This is also true if you start using css selectors to modify the internals of widgets. widgets and deep css selectors aren't really compatible concepts from the view of building a widget library.

Plus, if you make changes like spawning or despawning grid items after the initial spawn, the column alignment will break.

In the Credits example, and I think most grid examples, you would want to despawn a row of elements, not an individual cell. Despawning a cell in a grid always breaks the grid in this case because you presumably made the cells have specific layout behavior for a reason. So you either clear the cell and retain the node, or remove the whole row.

@benfrankel
Copy link
Contributor Author

benfrankel commented May 19, 2025

So you don't need the enumerate, etc and the implementation makes sense as paired output for each row.

That is a semantic improvement, but I don't think it's an ergonomic improvement. For example, consider what would have to change if you want to replace one of the text labels in the grid with an image.

From a widget/component lib design perspective: If a Widget's behavior is supposed to be modifiable by an end user, that needs to be part of its API.

I can understand this as a matter of principle, but I believe this is unfortunately not viable in practice with the current recommended spawning API. Any time you want to make an existing widget more flexible, you have to add a new argument to the function and update all the existing calls to it accordingly. And then you end up with unreadable code like this:

widget::label(Vw(2.5), Vw(4.0), Vw(3.4), Vw(5.6), 14.0, Color::WHITE, "my text")

Note that the (widget::label(...), Node { ... }) approach in the credits example only works because widget::label just so happens to use Node::default() implicitly via required components.

In the Credits example, and I think most grid examples, you would want to despawn a row of elements, not an individual cell.

Yeah that's probably fair. I wouldn't be surprised if there's some niche example where this isn't the case, but I don't have one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Feature A new feature, making something new possible S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Development

No branches or pull requests

3 participants