-
Notifications
You must be signed in to change notification settings - Fork 186
feat(protocol-designer): Update Shift-click step selection to work with concurrent Thermocycler profiles #20193
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
Conversation
e.g. clarify the difference between stepId and selectedStepId.
| export const getMetaSelectedSteps = ( | ||
| multiSelectItemIds: StepIdType[] | null, | ||
| stepId: StepIdType, | ||
| selectedStepId: StepIdType | null | ||
| priorMultiSelectedItemIds: StepIdType[] | null, | ||
| newlySelectedStepId: StepIdType, | ||
| priorSingleSelectedStepId: StepIdType | null | ||
| ): StepIdType[] => { |
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.
No behavioral changes in getMetaSelectedSteps(), just renaming parameters for clarity.
| const orderedVisibleSteps = orderedStepIds | ||
| .slice(startIndex, endIndex + 1) | ||
| .filter(stepId => stepVisibilities[stepId].isVisibleToUser) | ||
| return orderedVisibleSteps |
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 new .filter() is the actual behavioral change of this PR.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## edge #20193 +/- ##
==========================================
- Coverage 56.91% 56.85% -0.07%
==========================================
Files 3570 3573 +3
Lines 297737 298507 +770
Branches 42215 42326 +111
==========================================
+ Hits 169453 169709 +256
- Misses 128053 128567 +514
Partials 231 231
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jerader
left a comment
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.
just 1 thing i noticed: you can't duplicate the step if the thermocycler step is selected. You probably have a separate ticket for this though!
But otherwise, this looks good to me! nice 😎
|
Uhh whoa, no, that is weird. That definitely should work at this point, and I thought it was working a couple days ago. Maybe I screwed something up in a merge. Looking into it. |
|
Oh, teehee, it's just because this PR is based on an Thanks for the quick review and for noticing that! |
Overview
Protocol Designer lets the user Shift-click to select a range of steps in the timeline. Now that the timeline has nesting (concurrent Thermocycler profiles), we need to update that logic to account for some hazards when the range crosses the boundary of a nested group. This PR does that.
Closes EXEC-2060. See that ticket for a more full example of the problem we're solving.
Test Plan and Hands on Testing
I've done this:
Changelog
getShiftSelectedSteps()to avoid selecting steps that are not "user-visible"—i.e., the hidden "wait for profile to complete" step that implicitly comes at the end of every Thermocycler profile. Some of the other step CRUD actions are already doing the same thing.SelectMultipleStepsForGroupAction, which was unused (never created, nor used in any reducer).Review requests
OK with deleting
SelectMultipleStepsForGroupAction?Risk assessment
Low.