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

refactor(app, app-shell-odd): update configuration translation #13503

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

shlokamin
Copy link
Member

@shlokamin shlokamin commented Sep 8, 2023

Overview

Currently when running make -C app dev-odd you see the welcome to your opentrons flex pop up. This is because the makefile specifies the unfinished unboxing flow route to '/dashboard' What we want, really, is to set it to null, which indicates that the unboxing flow has been completed and we don't need any specific redirect.

The problem is that the make commands are run in the OS shell environment, not in a JS environment. This means there is no way to convey core JS data types like null. So, what i did for the dev odd target was pass in "0" as the unfinished unboxing flow route, and i have the selector that grabs app configs explicitly check for string "0" and convert it to null.

Don't love this, so if anyone has some cleaner way to go about it I'm all ears

Review requests

Run the dev-odd make command and make sure the welcome to your opentrons flex pop up does not show up

@shlokamin shlokamin marked this pull request as ready for review September 8, 2023 18:51
@shlokamin shlokamin requested review from a team as code owners September 8, 2023 18:51
@shlokamin shlokamin requested review from ncdiehl11, koji and b-cooper and removed request for a team September 8, 2023 18:51
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (a970141) 68.23% compared to head (542a8e2) 68.22%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13503      +/-   ##
==========================================
- Coverage   68.23%   68.22%   -0.01%     
==========================================
  Files        2514     2504      -10     
  Lines       71885    71488     -397     
  Branches     9179     9114      -65     
==========================================
- Hits        49048    48776     -272     
+ Misses      20668    20574      -94     
+ Partials     2169     2138      -31     
Flag Coverage Δ
app 64.81% <80.00%> (-0.01%) ⬇️
components 49.62% <ø> (ø)
labware-library 41.10% <ø> (ø)
protocol-designer 38.01% <ø> (ø)
react-api-client 66.16% <ø> (ø)
step-generation 87.18% <ø> (+0.27%) ⬆️

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

Files Coverage Δ
app/src/redux/config/selectors.ts 89.74% <80.00%> (-2.76%) ⬇️

... and 242 files with indirect coverage changes

@ecormany
Copy link
Contributor

ecormany commented Oct 6, 2023

The main behavior works as intended, but for some reason on this branch the window is the wrong size. It should be 1024x600 (@ 2x if on a high-resolution display) but instead it's 800x600.
Screenshot 2023-10-06 at 11 41 15 AM

This also has some weird text size knock-on effects:
Screenshot 2023-10-06 at 11 46 36 AM

@koji
Copy link
Contributor

koji commented Oct 6, 2023

@ecormany
You see the weird text size because right now we switch the text size by Windows size if the Window's size 1024x600, the app uses ODD text definition.
Probably we will add min-height (601px) to avoid seeing this text weirdness.

@koji
Copy link
Contributor

koji commented Oct 6, 2023

I tried to make -C dev-odd OT_APP_UI__WIDTH=1024 but the window's size wasn't changed.
However, make -C dev OT_APP_UI__WIDTH=1200 worked as expected.
Not sure why dev-odd is facing this issue.

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

✔️

@shlokamin
Copy link
Member Author

oh yeah this is an actual issue — i changed the library we use to parse command flags to NOT parse numbers as numbers which messing up setting the screen size. ill push up a fix

@shlokamin shlokamin merged commit 85d5007 into edge Feb 1, 2024
39 checks passed
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.

4 participants