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.

Sign up for GitHub

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

Jump to bottom

refactor(protocol-designer): refactor EditInstrumentsModal #17226

Merged
merged 12 commits into from
Jan 10, 2025
Merged

refactor(protocol-designer): refactor EditInstrumentsModal #17226

merged 12 commits into from
Jan 10, 2025

Conversation

koji
Copy link
Contributor

@koji koji commented Jan 8, 2025 •
edited
Loading

Overview

this is prep work for adding tests and resolving import warnings. PipetteConfig and PipetteOverview tests will be followed by another pr.
refactored EditInstrumentsModal
Export most parts and make EditInstrumentsModal as the main component

close RQA-3283 and RQA-3847

Test Plan and Hands on Testing

  • check EditInstrumentsModal works as expected

Changelog

Review requests

Risk assessment

low

@koji koji requested review from ncdiehl11 and jerader January 8, 2025 22:48
@koji koji added the authorship label Jan 8, 2025
@koji koji marked this pull request as ready for review January 8, 2025 22:53
@koji koji requested a review from a team as a code owner January 8, 2025 22:53
@koji koji removed the request for review from a team January 8, 2025 22:54
@koji koji marked this pull request as draft January 9, 2025 04:08
@koji
Copy link
Contributor Author

koji commented Jan 9, 2025 •
edited
Loading

  • create a custom hook for useStates

@koji koji marked this pull request as ready for review January 9, 2025 19:41
@jerader
Copy link
Collaborator

jerader commented Jan 10, 2025 •
edited
Loading

i don't think we should allow this state? looks like we allow it in production which we shouldn't...

Screenshot 2025-01-10 at 08 24 26

jerader
jerader reviewed Jan 10, 2025
View reviewed changes
protocol-designer/src/organisms/EditInstrumentsModal/usePipetteConfig.ts
resetFields: () => void
}

export const usePipetteConfig = (): PipetteConfig => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

jerader
jerader approved these changes Jan 10, 2025
View reviewed changes
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for adding tests. left a few clean up comments and posted a bug that is not introduced in this PR but should probably be addressed.

@koji
Copy link
Contributor Author

koji commented Jan 10, 2025

i don't think we should allow this state? looks like we allow it in production which we shouldn't...

Screenshot 2025-01-10 at 08 24 26

Screenshot 2025-01-10 at 11 23 42 AM

@koji koji merged commit 1ceba43 into edge Jan 10, 2025
14 checks passed
@koji koji deleted the refactor_edit-instruments-modal branch January 10, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerader jerader jerader approved these changes

@ncdiehl11 ncdiehl11 Awaiting requested review from ncdiehl11

Assignees
No one assigned
Labels
authorship
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@koji @jerader