-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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] Polish runs feed UI #25712
[ui] Polish runs feed UI #25712
Conversation
Deploy preview for dagit-storybook ready! ✅ Preview Built with commit d81f5a3. |
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit d81f5a3. |
1bd28b8
to
89187bc
Compare
@@ -25,8 +25,9 @@ export const PageHeader = (props: Props) => { | |||
> | |||
{title && ( | |||
<Box | |||
padding={{vertical: 8}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a global change but I verified it on a bunch of pages - the padding only takes effect when the height of the content in this box exceeds 36px, which is only the case when it is wrapping to two lines. It does need to be 8 or less pixels though -- otherwise our headers containing buttons get taller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth making this its own PR? It might be annoying to have to do a revert if this breaks something unexpectedly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I think you're right, I'll break this out into a separate PR
@@ -317,17 +317,19 @@ export const AssetNodeOverview = ({ | |||
</AttributeAndValue> | |||
|
|||
<AttributeAndValue label="Code location"> | |||
<Box flex={{direction: 'column'}}> | |||
<Box flex={{direction: 'column', alignItems: 'flex-start', gap: 8}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it would be nice to have a mini-alert component, these always feel too big for sidebars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree, there's at least one that appears in the asset graph's sidebar and I think in that scenario we place it at the top with no whitespace around it all. Will see if Josh has thoughts about this in a follow-up
@@ -25,8 +25,9 @@ export const PageHeader = (props: Props) => { | |||
> | |||
{title && ( | |||
<Box | |||
padding={{vertical: 8}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth making this its own PR? It might be annoying to have to do a revert if this breaks something unexpectedly.
@@ -317,17 +317,19 @@ export const AssetNodeOverview = ({ | |||
</AttributeAndValue> | |||
|
|||
<AttributeAndValue label="Code location"> | |||
<Box flex={{direction: 'column'}}> | |||
<Box flex={{direction: 'column', alignItems: 'flex-start', gap: 8}}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like it would be nice to have a mini-alert component, these always feel too big for sidebars.
@@ -104,6 +104,12 @@ export const RunsFeedRow = ({ | |||
flex={{direction: 'row', alignItems: 'center', wrap: 'wrap'}} | |||
style={{gap: '4px 8px', lineHeight: 0}} | |||
> | |||
{entry.__typename === 'PartitionBackfill' ? ( | |||
<Tag intent="none"> | |||
<span>Backfill</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
span
wrapper necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it's not! Will remove
@@ -11,9 +11,9 @@ import {gql, useQuery} from '../apollo-client'; | |||
import {PYTHON_ERROR_FRAGMENT} from '../app/PythonErrorFragment'; | |||
import {RunsFilter} from '../graphql/types'; | |||
|
|||
const PAGE_SIZE = 25; | |||
const PAGE_SIZE = 29; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 29?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh this is because of a bug on the graphql side that causes it to return 1 more row than requested. Let me see if i can find the root cause and fix it there so this is less janky
07654fe
to
456d431
Compare
456d431
to
d81f5a3
Compare
Summary & Motivation
Related: https://linear.app/dagster-labs/issue/FE-515/backfills-and-runs-consolidation-ui-frontend
This is a collection of small tweaks to improve the UI/UX of the runs feed pulled from this doc of dogfooding feedback: https://www.notion.so/dagster/new-Runs-page-1-9-dogfooding-12918b92e462807688fee0e644a34641
How I Tested These Changes
I tested all of these manually using the runs feed page and other impacted pages (for the middle truncate items)