-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Simplify messaging for sync canceled jobs #20999
Conversation
* Hide stats * Show cancelled icon when it's multiple attempts * Update attempts count to gray * Extract shared attempt functions into utils file * Cleanup component export and scss imports * Fix height glitch when opening and closing log
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.
Nothing blocking, but had a few questions/comments.
@@ -44,16 +44,18 @@ | |||
.attemptCount { |
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.
Nit: we could use the <Text/>
component in place of the div wrapping this text to remove the explicit font sizes.
} | ||
} | ||
|
||
border-bottom: variables.$border-thin solid transparent !important; |
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.
Out of curiosity -- is this due to <Row />
having its own border: none;
rule in the styled component and it was also broken before?
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.
@tealjulia Yep that's the reason.
}; | ||
|
||
const AttemptDetails: React.FC<AttemptDetailsProps> = ({ attempt, className, hasMultipleAttempts }) => { | ||
export const AttemptDetails: React.FC<AttemptDetailsProps> = ({ attempt, className, hasMultipleAttempts }) => { | ||
const { formatMessage } = useIntl(); | ||
|
||
if (attempt.status !== AttemptStatus.succeeded && attempt.status !== AttemptStatus.failed) { |
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.
Not related to these changes... but I'm curious why we don't just check if attempt.status === AttemptStatus.running
here. It doesn't look like it's ever null/undefined from the types.
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.
It just needs to be cleaned up because of changes that were made to get rid of the Status enum which was tracking too many statuses:
https://github.com/airbytehq/airbyte/pull/19599/files#diff-9438e96560c48e89bef8ea05895b60ab0cc824a87ae8408196d4de80a12b4f4aL24
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.
LGTM pending Teal's ✅
nice! looks great. |
* Hide stats * Show cancelled icon when it's multiple attempts * Update attempts count to gray * Extract shared attempt functions into utils file * Cleanup component export and scss imports * Fix height glitch when opening and closing log
What
Resolves #19630
Updates information when a Sync is canceled:
https://www.loom.com/share/26bdb2489be34416aa56e33709ed5a6c
How
Adds more checks. For attempts, it checks if the failure type was a manual cancelation
Misc:
Recommended reading order
From top to bottom