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, react-api-client): wire up deck configuration editor and add useCreateDeckConfigurationMutation #13817

Merged
merged 23 commits into from
Oct 27, 2023

Conversation

koji
Copy link
Contributor

@koji koji commented Oct 19, 2023

Overview

update deck configuration editor, add fixture modal, and deck config discard modal to align with expected behavior.

close RAUT-804

Test Plan

Changelog

  • add useCreateDeckConfigurationMutation and test (test will be added later)
  • update DeckConfiguration page component, DeckConfiguration for ProtocolSetup and AddFixtureModal to wire up Confirm button

Review requests

  • I added DeckConfigData to detect data changes for confirm button, but I feel that there might be more better way for doing that.
  • I change DeckConfiguration page component to DeckConfigurationEditor to avoid a name conflict but I didn't change the folder name since Rob is working on re-organizing Figma and at some point we will need to align components' names with design.
    [related thread]
    https://opentrons.slack.com/archives/CSCLVUW3C/p1698246368962059?thread_ts=1698246124.637669&cid=CSCLVUW3C
  • I created handleTapAdd in AddFixtureModal to avoid using if-statement since the behavior of Desktop and the behavior of ODD are slightly different. But at the same time maybe it would be good to keep using handleClickAdd for both cases?

Risk assessment

low

@koji
Copy link
Contributor Author

koji commented Oct 19, 2023

  • Deck configuration page component
    • update ODD part
    • update Desktop part
    • update tests
      • AddFixtureModal
      • DeckConfigurationDiscardChangesModal
      • DeckConfiguration page component
  • Deck configuration in ProtocolSetup (need to merge feat(app): add Deck configuration screen to ProtocolSetup #13808 into edge)
    • update ODD part
  • useCreateDeckConfigurationMutation
    • update test

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #13817 (2f7139f) into edge (072dced) will increase coverage by 0.02%.
Report is 25 commits behind head on edge.
The diff coverage is 56.75%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13817      +/-   ##
==========================================
+ Coverage   70.61%   70.63%   +0.02%     
==========================================
  Files        2448     2457       +9     
  Lines       67325    68855    +1530     
  Branches     8478     9111     +633     
==========================================
+ Hits        47542    48637    +1095     
- Misses      17781    18073     +292     
- Partials     2002     2145     +143     
Flag Coverage Δ
app 68.28% <51.85%> (+0.03%) ⬆️
react-api-client 65.82% <70.00%> (+0.09%) ⬆️

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

Files Coverage Δ
app/src/App/OnDeviceDisplayApp.tsx 76.59% <ø> (+2.52%) ⬆️
...iguration/DeckConfigurationDiscardChangesModal.tsx 100.00% <100.00%> (ø)
...nisms/ProtocolSetupModulesAndDeck/FixtureTable.tsx 85.18% <ø> (ø)
...organisms/ProtocolSetupDeckConfiguration/index.tsx 70.58% <80.00%> (+10.58%) ⬆️
...DeviceDetailsDeckConfiguration/AddFixtureModal.tsx 81.81% <50.00%> (-3.37%) ⬇️
...onfiguration/useCreateDeckConfigurationMutation.ts 70.00% <70.00%> (ø)
app/src/pages/DeckConfiguration/index.tsx 57.57% <35.71%> (-15.16%) ⬇️

... and 68 files with indirect coverage changes

@koji koji changed the title refactor(app): wire up deck configuration editor feat(app, react-api-client): wire up deck configuration editor and add Oct 20, 2023
@koji koji changed the title feat(app, react-api-client): wire up deck configuration editor and add feat(app, react-api-client): wire up deck configuration editor and add useCreateDeckConfigurationMutation Oct 20, 2023
@koji koji requested review from brenthagen, jerader and a team October 25, 2023 18:11
@koji koji marked this pull request as ready for review October 25, 2023 18:14
@koji koji requested a review from a team as a code owner October 25, 2023 18:14
@koji koji requested a review from a team as a code owner October 25, 2023 18:14
@koji koji removed request for a team October 25, 2023 21:00
Copy link
Contributor

@brenthagen brenthagen left a comment

Choose a reason for hiding this comment

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

some initial comments on handling deck config local state

Comment on lines 60 to 63
const [deckConfigData, setDeckConfigData] = React.useState<DeckConfigData>({
addedFixture: null,
currentDeckConfig: deckConfig,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

this local state should just be a DeckConfiguration - we don't need to pass an added fixture into state.

then, probably the way we should handle setting state (in add fixture modal) is to use an updater function to set state based on previous state. something like (psuedo):

previousState => {
// filter previous state by fixtureLocation
// append new fixture
// return new state
}

another option is useReducer but we probably don't need to go there for this

Copy link
Contributor

Choose a reason for hiding this comment

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

and handleClickRemove needs to be updated to use the local state config in a similar way, appending a standard slot to the local state

@@ -118,7 +145,7 @@ export function DeckConfiguration(): JSX.Element {
justifyContent={JUSTIFY_CENTER}
>
<DeckConfigurator
deckConfig={deckConfig}
deckConfig={deckConfigData.currentDeckConfig}
Copy link
Contributor

Choose a reason for hiding this comment

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

this data initializes to [] because the query is async - we need to:

  1. add a useEffect to update local state when the deck config query resolves
  2. probably block rendering the configurator (render null) when deck config length == 0 because current it renders as an empty deck map on load

Copy link
Contributor

Choose a reason for hiding this comment

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

Screen Shot 2023-10-26 at 10 45 30 AM

import type { ModalHeaderBaseProps } from '../../molecules/Modal/types'
import type { LegacyModalProps } from '../../molecules/LegacyModal'

interface AddFixtureModalProps {
fixtureLocation: Cutout
setShowAddFixtureModal: (showAddFixtureModal: boolean) => void
setCurrentDeckConfig?: React.Dispatch<React.SetStateAction<DeckConfiguration>>
Copy link
Contributor Author

@koji koji Oct 26, 2023

Choose a reason for hiding this comment

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

we need this change because the following gets typescript type error

setCurrentDeckConfig: (currentDeckConfig: DeckConfiguration) => void

@koji koji requested a review from brenthagen October 26, 2023 20:43
Copy link
Contributor

@brenthagen brenthagen left a comment

Choose a reason for hiding this comment

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

just some routing comments, good to merge after those addressed ✔️

@koji koji merged commit 17a4e6f into edge Oct 27, 2023
22 of 23 checks passed
caila-marashaj pushed a commit that referenced this pull request Oct 31, 2023
…d useCreateDeckConfigurationMutation (#13817)

*feat(app, react-api-client): wire up deck configuration editor and add useCreateDeckConfigurationMutation
@koji koji deleted the app_refactor-wireup-deckconfiguration-confirmbutton branch November 29, 2023 04:33
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