Skip to content

feat(app): Add default offset info banner to LPC#17923

Merged
mjhuff merged 6 commits intochore_release-8.4.0from
app_add-default-offset-info-lpc
Mar 28, 2025
Merged

feat(app): Add default offset info banner to LPC#17923
mjhuff merged 6 commits intochore_release-8.4.0from
app_add-default-offset-info-lpc

Conversation

@mjhuff
Copy link
Copy Markdown
Contributor

@mjhuff mjhuff commented Mar 28, 2025

Closes EXEC-1392 and RQA-4018

Overview

After QA feedback, Design has decided to add an info banner that includes an explanation of default offsets. The banner should persist for one LPC wizard session, so this is state is kept in the general wizard container.

Note that designs show tooltips for the desktop app, but after convo with Design, we're just doing the banner for both.

Screen.Recording.2025-03-28.at.12.29.21.PM.mov
Screenshot 2025-03-28 at 1 20 21 PM

Test Plan and Hands on Testing

  • See screenshot.

Changelog

  • Added an info banner containing an explanation for default offsets.

Risk assessment

low

@mjhuff mjhuff requested review from a team, SyntaxColoring and sfoster1 March 28, 2025 17:49
@mjhuff mjhuff requested a review from a team as a code owner March 28, 2025 17:49
@mjhuff mjhuff requested review from koji and removed request for a team and koji March 28, 2025 17:49
@mjhuff mjhuff force-pushed the app_add-default-offset-info-lpc branch from b9a4753 to 039f25c Compare March 28, 2025 18:14
Copy link
Copy Markdown
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

🟦

Comment on lines +12 to +13
// Holds state & functionality for managing banners that require app-wide persistent state.
// If state should persist between LPC wizard sessions, use Redux instead!
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: this comment is a little confusing to me because "app-wide persistent state" sounds to me like the same thing as "state that should persist between LPC wizard sessions."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Separately, for my edification, why not use Redux for this? Just overkill?

Copy link
Copy Markdown
Contributor Author

@mjhuff mjhuff Mar 28, 2025

Choose a reason for hiding this comment

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

Sorry, yeah, that comment is a bit confusing. I'll update it.

Redux is honestly fine, good point. We'd have to reset the banner state when we close the app for the correct behavior, which seems a bit clunky to me, but I still might throw it in Redux just to keep things consistent.

EDIT: Due to time constraints, I've left a TODO here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We'd have to reset the banner state when we close the app for the correct behavior

Oh whoa, does our redux state persist across app launches?

Copy link
Copy Markdown
Contributor Author

@mjhuff mjhuff Mar 28, 2025

Choose a reason for hiding this comment

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

Sorry, confusing response on my part. I meant when we close the LPC wizard flows, NOT the app, since LPC state is persisted for a given protocol run indefinitely as long as the app is open. Closing the app definitely resets state (at least all the state that is not persisted in config)!

@mjhuff mjhuff force-pushed the app_add-default-offset-info-lpc branch from 855026d to dd4d534 Compare March 28, 2025 19:38
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 28, 2025

Codecov Report

Attention: Patch coverage is 47.19101% with 47 lines in your changes missing coverage. Please review.

Project coverage is 25.68%. Comparing base (e1eac6f) to head (2d44ec2).
Report is 8 commits behind head on chore_release-8.4.0.

Files with missing lines Patch % Lines
...nisms/LabwarePositionCheck/hooks/useInfoBanners.ts 0.00% 16 Missing ⚠️
app/src/molecules/MultiDeckLabelTagBtns/index.tsx 0.00% 13 Missing ⚠️
...ck/steps/HandleLabware/LPCLabwareDetails/index.tsx 0.00% 10 Missing ⚠️
...c/organisms/LabwarePositionCheck/LPCWizardFlex.tsx 0.00% 3 Missing ⚠️
...ePositionCheck/__fixtures__/mockLPCContentProps.ts 0.00% 3 Missing ⚠️
...ionCheck/LPCFlows/hooks/useLPCLabwareInfo/index.ts 0.00% 1 Missing ⚠️
.../src/organisms/LabwarePositionCheck/hooks/index.ts 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-8.4.0   #17923      +/-   ##
=======================================================
- Coverage                25.82%   25.68%   -0.14%     
=======================================================
  Files                     3005     3014       +9     
  Lines                   227433   229241    +1808     
  Branches                 18912    18993      +81     
=======================================================
+ Hits                     58724    58884     +160     
- Misses                  168696   170344    +1648     
  Partials                    13       13              
Flag Coverage Δ
protocol-designer 18.68% <47.19%> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...rc/organisms/LabwarePositionCheck/types/content.ts 100.00% <100.00%> (ø)
components/src/atoms/buttons/SecondaryButton.tsx 100.00% <100.00%> (ø)
...ionCheck/LPCFlows/hooks/useLPCLabwareInfo/index.ts 0.00% <0.00%> (ø)
.../src/organisms/LabwarePositionCheck/hooks/index.ts 0.00% <0.00%> (ø)
...c/organisms/LabwarePositionCheck/LPCWizardFlex.tsx 0.00% <0.00%> (ø)
...ePositionCheck/__fixtures__/mockLPCContentProps.ts 0.00% <0.00%> (ø)
...ck/steps/HandleLabware/LPCLabwareDetails/index.tsx 0.00% <0.00%> (ø)
app/src/molecules/MultiDeckLabelTagBtns/index.tsx 0.00% <0.00%> (ø)
...nisms/LabwarePositionCheck/hooks/useInfoBanners.ts 0.00% <0.00%> (ø)

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mjhuff mjhuff force-pushed the app_add-default-offset-info-lpc branch from 4d8cebf to 557bac9 Compare March 28, 2025 20:30
@mjhuff mjhuff requested a review from a team as a code owner March 28, 2025 20:46
@mjhuff mjhuff force-pushed the app_add-default-offset-info-lpc branch from 265899c to 2d44ec2 Compare March 28, 2025 21:10
@mjhuff mjhuff merged commit b0d9f72 into chore_release-8.4.0 Mar 28, 2025
38 checks passed
@mjhuff mjhuff deleted the app_add-default-offset-info-lpc branch March 28, 2025 21: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.

2 participants