Skip to content
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

feat(app): add DeckInfoLabelTextTag component #17605

Merged
merged 5 commits into from
Feb 28, 2025

Conversation

mjhuff
Copy link
Contributor

@mjhuff mjhuff commented Feb 27, 2025

Closes EXEC-1243

Overview

Adds the DeckInfoLabelTextTag component per designs. Breakpoints here.

NOTE: The Designs at the above file insinuate that the background-color is grey, but that's not actually the case.

Screenshot 2025-02-27 at 1 04 09 PM

ODD

Screenshot 2025-02-27 at 1 05 40 PM

Desktop

Test Plan and Hands on Testing

Changelog

  • Added new DeckInfoLabelTextTag component.

Risk assessment

low

@mjhuff mjhuff requested review from koji, sfoster1, ncdiehl11, jerader and a team February 27, 2025 18:06
@mjhuff mjhuff requested a review from a team as a code owner February 27, 2025 18:06
@mjhuff mjhuff force-pushed the app_add-deckinfolabeltexttag-component branch 3 times, most recently from ef7d6f9 to af0bf55 Compare February 27, 2025 21:34
@Opentrons Opentrons deleted a comment from codecov bot Feb 28, 2025
grid-template-columns: 1fr 1fr 1fr;
}

@media (max-width: 450px) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you tell me why this uses 450px?
Screenshot 2025-02-28 at 3 03 38 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we decided not to use the intermediate breakpoint, and Design is making the assumption that text will be 4 characters long, which in practice is never going to be the case by a long shot. Using 450px is more reasonable than 177px. I'm talking to her about this when she gets back and can put up a fix if necessary.

Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

thank you for the explanation!

@mjhuff
Copy link
Contributor Author

mjhuff commented Feb 28, 2025

lgtm

thank you for the explanation!

Of course. Thanks for taking the time to review all these PRs!

@mjhuff mjhuff merged commit 0536603 into edge Feb 28, 2025
40 checks passed
@mjhuff mjhuff deleted the app_add-deckinfolabeltexttag-component branch February 28, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants