-
Notifications
You must be signed in to change notification settings - Fork 178
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
fix(protocol-designer): liquid preservation in zoomed in slot and x button #16722
Conversation
const selectedSlotInfoRef = useRef<ZoomedIntoSlotInfoState>(selectedSlotInfo) | ||
// initialize the selectedSlotInfoRef | ||
useEffect(() => { | ||
selectedSlotInfoRef.current = selectedSlotInfo | ||
}, [selectedSlotInfo]) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason selectedSlotInfoRef
wasn't correctly showing the initial state that selectedSlotInfo
was showing so i had to add a useEffect to initialize it. I did some debugging and tried memoizing but that didn't work. Open to other ideas anyone has!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe creating a function or
use const selectedSlotInfo = useSelector(selectors.getZoomedInSlotInfo)
in DeckSetupContainer and pass selectedSlotInfo
as a prop?
function useUpdatedRef<T>(value: T): MutableRefObject<T> {
const ref = useRef(value)
if (!isEqual(ref.current, value)) {
ref.current = value
}
return ref
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
omg that is a great idea, thank you!!!
could not add a module and a labware to a slot and could not change a labware test.mov |
@koji oh wow, thanks for catching this! let me see if i can fix it before EOD |
// Check if initialSelectedSlotInfo is null and if selectedSlotInfo is | ||
// fully populated since component rerenders | ||
// TODO: need to optimize this better, find out why component rerenders |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to fix the bug. I think we need to refactor DeckSetupContainer
, DeckSetupTools
and DeckSetupDetails
and how it gets and keeps track of the initial slot information. If you log anything in DeckSetupTools
, you'll see it get re-rendered a bunch of times. Part of the re-rendering is due to the animation of zooming in. I guess what we have now is fine but definitely should be revisited and refactored post-release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
closes AUTH-1016, partially addresses RQA-3526
Overview
Oof this was a bit confusing but i think the liquid edit experience is fixed now when you edit a labware with liquids in it already.
Test Plan and Hands on Testing
First check that the X button is added to deck setup tools and works as expected
Then, zoom into a slot with liquids and click done without doing anything, the liquids should be preserved until you press done
Next, zoom into a slot with liquids and click on different labware, liquids should not be preserved across labware but SHOULD be preserved if you go back to the original labware. once you press done, the liquids are not preserved.
Check that this pattern works for labware on the deck, labware on an adapter, labware on a module, and labware on an adapter on a module
here is a test protocol:
1.0 testing.json
Changelog
Risk assessment
low