-
Notifications
You must be signed in to change notification settings - Fork 0
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
X1062 probe hybridisation qc #377
Conversation
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.
Cool, looking good.
Just one question , the story says 'The labware must have had probe hybridisation already recorded on it.'. Of course the core will return terror, but would it be better to let user know as soon as they scan labware?
path={'/lab/probe_hybridisation_qc'} | ||
icon={<LabwareIcon className="flex-shrink-0 h-6 w-6 text-sdb-400" />} | ||
description={'Recording Probe hybridisation QC completion date and set sample sections comments.'} | ||
/> |
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.
There is a section below to display menu items for smaller screen width. Would you please update this menu item there as well?
src/pages/ProbeHybridisationQC.tsx
Outdated
labwares: { | ||
'': { | ||
globalComments: [], | ||
completionDateTime: '', |
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.
completionDateTime can be initialised to the current time
src/pages/ProbeHybridisationQC.tsx
Outdated
slot.samples.forEach((sample) => { | ||
const sampleAddressId = `${slot.address}-${sample.id}`; | ||
const oldSelected = values.labwares[labware.barcode]?.sampleAddressComments?.[sampleAddressId] || []; | ||
const updatedSelected = [...oldSelected, ...mapCommentOptionsToValues(options)]; |
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.
Why we need oldSelected and updateSelected?
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.
to cover the use case; where the user selects some options for a specific section and then adds other comments from the global 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.
Ohh, is it supposed to behave like that or is it to override everything by comments from 'apply all' drop down when they change some value in it ? It is better to clarify with Katy.
src/pages/ProbeHybridisationQC.tsx
Outdated
}, []); | ||
|
||
const updateSectionCommentsFromGlobal = useCallback( | ||
(options: OptionType[], labware: LabwareFieldsFragment, values, setFieldValue) => { |
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.
It would be good to specify values ad setFieldValue as it defaults to 'any' in typescript which should generally be avoided whenever possible
<AppShell.Main> | ||
<Formik<ProbeHybridisationQCFormValues> | ||
initialValues={formInitialValues} | ||
validationSchema={validationWorkNumber} |
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.
You are not validating time field which is required
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.
the validation is done automatically by the input field, as I specified the min/max there
src/pages/ProbeHybridisationQC.tsx
Outdated
/> | ||
<MutedText className="pl-2"> | ||
If not manually selected, the current timestamp will be applied automatically | ||
</MutedText> |
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.
Please make use of formik error handling capabilities, by including this field in validation schema , so we can avoid using MutedText and validateCompletionDateTime
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.
hmmm I am not sure how to include this in the formik errors as the request is still valid, when the completion time is not been selected
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.
I think the request should not be valid with taht empty field. It can display a default value in the field which is the current time when it opens, and the validation can be based on that time which is the default
} | ||
) | ||
); | ||
}); |
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.
Please add a global mock handler for this as we do with all other operations in stan-client
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.
Story says , 'Probe hybridisation comments (editable on config page)' . i think this need to be added in config page
src/components/Routes.tsx
Outdated
key={routeProps.location.key} | ||
dataFetcher={() => | ||
stanCore.GetComments({ | ||
commentCategory: 'section', |
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 should not be 'section', but Probe hybridisation comments I hope
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.
Good point, as the comments are applied for section, I thought we need to get the section comments?do you think we need tp get the Probe QC comment instead ?
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.
Good point, as the comments are applied for section, I thought we need to get the section comments?do you think we need tp get the Probe QC comment instead ?
I understood it that way which need to be added in configuration page as well. Better to clarify it with Dave or Minal/Katy.
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.
It seems like are some unintended changes happened with merge. Please check locally that everything works fine with your page and Xenium Analyser page
}) | ||
); | ||
}) | ||
) |
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.
I think this needs to be in this file
@@ -67,9 +112,20 @@ describe('Probe Hybridisation QC', () => { | |||
}); | |||
|
|||
describe('Submission', () => { | |||
context('when selecting a released labware', () => { | |||
context('when repeated comment specified for the same section', () => { |
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.
The context name doesn't match with what it does
@@ -266,7 +266,7 @@ describe('Xenium Probe Hybridisation', () => { | |||
}); | |||
|
|||
it('shows a success message', () => { | |||
cy.findByText('Xenium probe hybridisation recorded on all labware').should('be.visible'); | |||
cy.findByText('Xenium probe hubridisation recorded on all labware').should('be.visible'); |
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.
Something gone wring with merge it seems. this typo was fixed in the other branch which is again reverted back now. Please change to 'hybridisation'
path={'/lab/xenium_analyser'} | ||
icon={<LabwareIcon className="flex-shrink-0 h-6 w-6 text-sdb-400" />} | ||
description={'Recording Xenium analyser information'} | ||
description={'Recording Probe hybridisation for Xenium slides.'} |
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.
Again merge - The previous description was correct 'Recording Xenium analyser information'
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.
@sabrine33 did you change this? I couldn't see this update in your PR
<div className="grid grid-cols-2 ml-2 gap-y-4 gap-x-8"> | ||
<StanMobileNavLink to="/lab/xenium_analyser">Xenium Analyser</StanMobileNavLink> |
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 shouldn't be removed
src/components/Table.tsx
Outdated
}; | ||
export const TabelCentredCell: React.FC<Props> = ({ children, ...rest }) => { | ||
return <div className="flex items-center justify-center">{children}</div>; | ||
}; |
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 as well need to be kept
@@ -1,5 +1,5 @@ | |||
query FindLatestOperation($barcode: String!, $operationType: String!) { | |||
findLatestOp(barcode: $barcode, operationType: $operationType) { | |||
id | |||
...OperationFields |
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 needs to be id not OperationFields
path={'/lab/xenium_analyser'} | ||
icon={<LabwareIcon className="flex-shrink-0 h-6 w-6 text-sdb-400" />} | ||
description={'Recording Xenium analyser information'} | ||
description={'Recording Probe hybridisation for Xenium slides.'} |
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.
@sabrine33 did you change this? I couldn't see this update in your PR
…e_hybridisation_QC # Conflicts: # src/components/Routes.tsx # src/mocks/repositories/commentRepository.ts # src/types/sdk.ts
No description provided.