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): Show module name over labware on deckmaps #1913

Merged
merged 2 commits into from
Jul 23, 2018

Conversation

mcous
Copy link
Contributor

@mcous mcous commented Jul 23, 2018

overview

This is the last remaining PR to close out the current sprint's modules tickets. It adds a module name overlay to any labware that are sitting on modules on the deck.

Closes #1739

screenshot 2018-07-23 14 52 43

screenshot 2018-07-23 14 52 53

changelog

  • feat(app): Show module name over labware on deckmaps

review requests

Follow instructions in #1910 to stub out protocol (session) and actual modules and enable module with OT_APP_MODULES=1. Then:

  • Module name overlay appears in labware review modal deckmap
  • Module name overlay appears in labware calibration deckmap
  • If OT_APP_MODULES is unset or set to 0, no modules appear anywhere

Per discussion with @pantslakz over Slack, these do not line up with screens because:

  1. Modifying existing ContainerNameOverlay to support module name would've been frustrating
  2. ContainerNameOverlay is likely going to be refactored with the rest of the Deck components
  3. Screens are still subject to change once we can start UX testing in the wild

@mcous mcous added feature Ticket is a feature request / PR introduces a feature app Affects the `app` project ready for review labels Jul 23, 2018

{!showSpinner && module && (
// TODO(mc, 2018-07-23): displayName?
<ModuleNameOverlay name={module.name} width={width}/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

width prop unnecessary

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.

⚙️

@codecov
Copy link

codecov bot commented Jul 23, 2018

Codecov Report

Merging #1913 into edge will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge    #1913      +/-   ##
==========================================
- Coverage   32.89%   32.86%   -0.03%     
==========================================
  Files         406      407       +1     
  Lines        6637     6643       +6     
==========================================
  Hits         2183     2183              
- Misses       4454     4460       +6
Impacted Files Coverage Δ
...src/components/ReviewDeckModal/LabwareComponent.js 0% <0%> (ø) ⬆️
app/src/components/DeckMap/index.js 0% <0%> (ø) ⬆️
app/src/components/DeckMap/ModuleNameOverlay.js 0% <0%> (ø)
app/src/components/DeckMap/ConnectedSlotItem.js 0% <0%> (ø) ⬆️
app/src/components/DeckMap/LabwareItem.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2e63e3...d4db61e. Read the comment docs.

Copy link
Contributor

@IanLondon IanLondon left a comment

Choose a reason for hiding this comment

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

@mcous mcous merged commit c40905b into edge Jul 23, 2018
@mcous mcous deleted the app_module-name-over-slot branch July 23, 2018 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app Affects the `app` project feature Ticket is a feature request / PR introduces a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modules: View in Deck Map
3 participants