-
Notifications
You must be signed in to change notification settings - Fork 178
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
fix(app, odd): run summery error list bug fixes #15953
fix(app, odd): run summery error list bug fixes #15953
Conversation
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.
Missed this before, but these texts should be in an i18next lookup with its built-in pluralization logic
@@ -43,7 +43,6 @@ export function ModalHeader(props: ModalHeaderBaseProps): JSX.Element { | |||
color={iconColor} | |||
size="2rem" | |||
alignSelf={ALIGN_CENTER} | |||
marginRight={SPACING.spacing16} |
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.
really? this will affect all the rest of the modals on the ODD
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.
we have a gap in the div above it. I think we can remove this. can test more if needed!
app/src/organisms/OnDeviceDisplay/RunningProtocol/RunFailedModal.tsx
Outdated
Show resolved
Hide resolved
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.
Internationalization still not quite right I'm afraid
@@ -95,7 +107,9 @@ export function RunFailedModal({ | |||
errorType: errors[0].errorType, | |||
errorCode: errors[0].errorCode, | |||
}) | |||
: `${errors.length} errors`} | |||
: `${errors.length} ${ | |||
runStatus === RUN_STATUS_SUCCEEDED ? t('warning') : t('error') |
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 won't translate properly, right? if you translated it into say chinese, if there was more than one error it would have an "s" at the end of the chinese characters, and because the translation isn't number-aware it wouldn't have the right counting word. we should be using the i18n pluralization tools here
title: | ||
commandErrorList == null || commandErrorList?.data.length === 0 | ||
? t('run_failed_modal_title') | ||
: runStatus === RUN_STATUS_SUCCEEDED |
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.
same thing re: plural details
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.
approving to unblock, but we should do the pluralization thing
we probably need to update the packages. will do this in another pr/release. for now reverted the plural changs. |
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.
Nice! I haven't had the time to test it myself yet, but the code looks good.
Feel free to merge after CI passes. Thank you for the changes!
Overview
Closes RQA-2923.
show warnings/errors depending on the run status.
modal header.
show error details on ODD if run completed with errors.
Test Plan and Hands on Testing
Changelog
RunFailedModal
.Risk assessment
low. bug fixes.