-
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): Properly truncate ODD command text #17003
Conversation
const TEXT_TRUNCATION_STYLE = css` | ||
display: -webkit-box; | ||
-webkit-line-clamp: 2; | ||
-webkit-box-orient: vertical; | ||
overflow: hidden; | ||
text-overflow: ellipsis; | ||
|
||
@media ${RESPONSIVENESS.touchscreenMediaQuerySpecs} { | ||
font-size: ${TYPOGRAPHY.fontSize22}; | ||
} | ||
` | ||
|
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.
maybe adding this as a style-constants?
like
export const TEXT_TRUNCATION_STYLE = (
lineClamp: number
): FlattenSimpleInterpolation => css`
display: -webkit-box;
-webkit-box-orient: vertical;
overflow: ${OVERFLOW_HIDDEN};
text-overflow: ellipsis;
word-wrap: break-word;
-webkit-line-clamp: ${lineClamp};
word-break: break-all; // for a non word case like aaaaaaaa
@media ${RESPONSIVENESS.touchscreenMediaQuerySpecs} {
font-size: ${TYPOGRAPHY.fontSize22};
}
`
pd is use this
https://github.com/Opentrons/opentrons/blob/edge/protocol-designer/src/atoms/constants.ts
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.
Because the style is local to RecoverySplash
, and we want the lineclamp
to always be 2
, I think hardcoding it is ok.
That being said, since it sounds like we use this in PD + elsewhere in the app, maybe we could do as you say in some more centralized location, perhaps moving more in the direction of style factories? Maybe we can talk about this in the next FE guild meeting!
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 good, agree that this is fiddly enough that it would be nice to have it as a constant somewhere.
Closes RQA-3713 and RQA-3712
Overview
This PR fixes ODD command text truncation issues.
cff8694 - We had text truncation, but this got lost in a recent refactor. Instead of just hard truncating the string at a certain number of characters, this truncates after two lines, which has the added benefit of working nicely on the desktop app, too!
17200d7 - Given the way the styling works around the command text here, we need to specify line height to ensure text actually truncates.
Current Behavior
Fixed Behavior
Test Plan and Hands on Testing
Changelog
Risk assessment
low