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

FlexLayout: Allow SceneFlexLayout to be child of another flex layout #135

Merged
merged 5 commits into from
Apr 11, 2023

Conversation

dprokop
Copy link
Member

@dprokop dprokop commented Apr 6, 2023

This allows SceneFlexLayout to be used as a child of another SceneFlexLayout, without the need for wrapping it with SceneFlexItem. Tried to make SceneFlexLayout to implement SceneFlexItemLike interface, but it's quite tricky since the primarily because the body and children differentiation.

@dprokop dprokop requested a review from torkelo April 6, 2023 12:15
<item.Component key={item.state.key} model={item} />
))}
{children.map((item) => {
if (isSceneFlexLayout(item)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awkward, but don't see any other way (apart from tsignore) to make TS happy

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dprokop I am a bit confused as well, this should not be needed as this component does not care about children or body.

I was able to make this work here, just need to simplify SceneFlexItemLikeState so that it does not include body
https://github.com/grafana/scenes/compare/flex-layout-children...flex-item-like-state?expand=1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tempted to remove SceneLayoutItemState as it just adds a bit of confusing (just specifies body, but can have this as a convention instead for SceneFlexItem and SceneGridItem )

@@ -45,9 +45,12 @@ function SceneFlexLayoutRenderer({ model }: SceneComponentProps<SceneFlexLayout>

return (
<div style={style}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is missing getFlexItemItemStyles(parent.state.direction || 'row', model);

If we are to use a SceneFlexLayout as an item it needs to behave as one as well

@dprokop
Copy link
Member Author

dprokop commented Apr 6, 2023

Thanks for having a look @torkelo. Did plenty of changes (SceneLayoutItemState removal included) so that it's I think slightly better now :)

@@ -43,16 +40,23 @@ function SceneFlexLayoutRenderer({ model }: SceneComponentProps<SceneFlexLayout>
minHeight: 0,
};

if (parent && isSceneFlexLayout(parent)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can just use instanceof SceneFlexLayout here

@dprokop dprokop merged commit 2ddafec into main Apr 11, 2023
@dprokop dprokop deleted the flex-layout-children branch April 11, 2023 07:43
@grafanabot
Copy link
Contributor

🚀 PR was released in v0.4.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants