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

[ui] Fix vertical centering of arrows on the asset graph, asset name wrapping #25974

Merged
merged 2 commits into from
Nov 19, 2024

Conversation

bengotow
Copy link
Collaborator

@bengotow bengotow commented Nov 18, 2024

Summary & Motivation

This fixes a couple specific issues with the asset graph and updates our AssetNode stories:

  • Lower the truncation threshold for asset names to prevent wrapping. I think the previous value 38 must have been for a smaller font size, or for a previous asset node width. I verified the current font size (14px) is what is specified in Josh's latest designs and added a story for this case.
image
  • Update the getAssetNodeDimensions function to reflect what is actually rendered and ensure the AssetNodeBox is always centered in the layout box so that horizontal edges hit the middle of the box. To make this easier I updated the storybooks to show the overall box in a gray color and draw a midline where edges will arrive. I think it'd been a while since we updated this and in a bunch of cases there was too much padding at the bottom of the box, which we were compensating for by giving asset groups a negative margin.

How I Tested These Changes

image
  • A few new storybook cases covering kind tags and combinations of checks + partitions + change reasons.

  • app-proxy testing against elementl prod's global graph, both in horizontal and vertical mode and looking at maximally-tall assets at the bottom of asset groups:

image

Copy link

github-actions bot commented Nov 18, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-d6tq87j0v-elementl.vercel.app
https://bengotow-2024-11-FE-481.core-storybook.dagster-docs.io

Built with commit c9564c3.
This pull request is being automatically deployed with vercel-action

<div
style={{
position: 'relative',
width: dimensions.width,
height: dimensions.height,
background: `linear-gradient(to bottom, rgba(100,100,100,0.15) 49%, rgba(100,100,100,1) 50%, rgba(100,100,100,0.15) 51%)`,
Copy link
Member

Choose a reason for hiding this comment

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

I know it's just Storybook, but does this look okay in light mode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh let me double check, that's a good question...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image Looks good!

@bengotow bengotow merged commit 2616e04 into master Nov 19, 2024
2 checks passed
@bengotow bengotow deleted the bengotow-2024-11/FE-481 branch November 19, 2024 20:36
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