-
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] FIx backfill partition range coloring, clarify meaning of progress bars #26123
Conversation
9bd2a6c
to
456ee08
Compare
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 307de04. |
456ee08
to
4afe744
Compare
const pctSucceeded = (100 * succeeded) / targeted; | ||
const pctFailed = (100 * failed) / targeted; | ||
const pctInProgress = (100 * inProgress) / targeted; |
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.
Worth truncating the decimal value so you don't end up with super long decimal values?
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 good call, I'll throw a .toFixed(2) on these when they're passed to the template grid CSS below just to keep it sane
1381288
to
6bf5582
Compare
<div style={{background: Colors.accentRed()}} /> | ||
<div style={{background: Colors.accentBlue()}} /> | ||
</div> | ||
<Caption color={Colors.textLight()}>{`${pctFinal}% completed`}</Caption> |
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.
Earlier in this PR, the terminology was standardized from "completed" to "succeeded". To maintain this consistency, consider updating the caption text to:
<Caption color={Colors.textLight()}>{`${pctFinal}% succeeded`}</Caption>
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
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 subtle but this is actually the place we want to disambiguate apart from succeeded
6bf5582
to
307de04
Compare
Summary & Motivation
Related: https://linear.app/dagster-labs/issue/FE-689/backfill-ui-partition-range-display-issue
This is a handful of small fixes for the backfill partitions pages:
These tags were rendering full-width, and the terminology (succeeded vs completed) was inconsistent. Clicking it takes you to a view of Succeeded runs, so I changed both to succeeded:
On this page, the two gray colors used in the bars were very similar, which was confusing - now using
backgroundDisabled()
:How I Tested These Changes
I tested these changes manually using both op and asset backfills!