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

Asset checks UI revamp #19934

Conversation

salazarm
Copy link
Contributor

Summary & Motivation

https://www.figma.com/file/1aaOz2e8PiCOGPGF82vsvw/Asset-Checks?node-id=749%3A1840&mode=dev

Things omitted from the designs:

  • Durations because we don't support that on the backend atm
  • Partitions also because we don't support that yet
  • Check dependencies, also because we don't support that yet

How I Tested These Changes

Screenshot 2024-02-20 at 11 56 50 PM Screenshot 2024-02-20 at 11 53 26 PM Screenshot 2024-02-20 at 11 52 37 PM

@salazarm salazarm requested a review from hellendag February 21, 2024 23:42
Copy link
Collaborator

@bengotow bengotow left a comment

Choose a reason for hiding this comment

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

This looks awesome! Left a few inline comments but nothing blocking 🙌

children: React.ReactNode;
isInitiallyCollapsed?: boolean;
}) => {
const [isCollapsed, setIsCollapsed] = React.useState(isInitiallyCollapsed);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm does this one look like the new Asset Overview one? (the "Description" header here: https://www.figma.com/file/wQHGZ2cjfpna2P2OaM4aTs/aset-catalog-ui?type=design&node-id=208-45868&mode=design&t=IUz5OJkO0QRbYlOv-0)

Maybe we can merge this one and that together. I'd left it in the AssetNodeOverview.tsx file because there weren't any other collapsing sections like that in the product, but maybe there are now!

default:
assertUnreachable(status);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ahh this is a much better place for these 👍

// This will effectively give us a height of 0
const {viewport, containerProps} = useViewport();
return (
<Box flex={{grow: 1}} {...containerProps} style={{position: 'relative'}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not 100% sure it's relevant, but it sounds like this could be a flexbox setup where the parent has justify-items: stretch, pulling all the children to the height of the tallest sibling, and this container box has min-height: 0, giving it no intrinsic content size of it's own?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let me try that! Tbh my flex game has been pretty weak but that sounds like what I want.

Copy link
Contributor Author

@salazarm salazarm Feb 23, 2024

Choose a reason for hiding this comment

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

Looks like justify-items stretch doesn't do anything in flex box, I think it's basically flex grow?

@salazarm salazarm merged commit ecba104 into master Feb 23, 2024
0 of 3 checks passed
@salazarm salazarm deleted the marco/fe-170-asset-checks-ui-20-converted-to-asset-checks-ui-20-project branch February 23, 2024 12:25
@johannkm
Copy link
Contributor

🙌

PedramNavid pushed a commit that referenced this pull request Mar 28, 2024
## Summary & Motivation


https://www.figma.com/file/1aaOz2e8PiCOGPGF82vsvw/Asset-Checks?node-id=749%3A1840&mode=dev

Things omitted from the designs: 

- Durations because we don't support that on the backend atm
- Partitions also because we don't support that yet
- Check dependencies, also because we don't support that yet

## How I Tested These Changes

<img width="2560" alt="Screenshot 2024-02-20 at 11 56 50 PM"
src="https://github.com/dagster-io/dagster/assets/2286579/842ef87f-2a4f-4844-a218-d848c0e89e17">
<img width="2558" alt="Screenshot 2024-02-20 at 11 53 26 PM"
src="https://github.com/dagster-io/dagster/assets/2286579/5940dae0-731d-4497-91d4-32b9053a953a">
<img width="2560" alt="Screenshot 2024-02-20 at 11 52 37 PM"
src="https://github.com/dagster-io/dagster/assets/2286579/212f6ca4-43f7-401e-b822-53f804eeab77">
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 this pull request may close these issues.

3 participants