DPL-954: colour code wells on arraying page #1580
Conversation
| C1: { pool_index: 3 }, | ||
| D1: { pool_index: 4 }, | ||
| C1: { pool_index: 2 }, | ||
| D1: { pool_index: 3 }, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Wondered if there were tests for the individual Vue components.
Also wondered what happens when they remove the last instance of a barcode from the scan boxes, does it free up that colour index to be used for the next new unique barcode?
There was a problem hiding this comment.
It probably shouldn't right, if that one they delete was in the middle of the colour list you don't want complications with latter ones being re-coloured.
There was a problem hiding this comment.
The Vue component testing is a little shaky right now, but that aside for the moment, the colours are calculated from the order that the barcodes are entered at any one moment. Deleting the last barcode will allow that colour to be reused on the subsequent barcode. Deleting the barcode will cause all other codes to be shifted up by one. This is currently a technical limitation, but could be altered if we decide it's worth the extra effort.
There was a problem hiding this comment.
I've added tests in 9735757 to show the behaviour of changing barcodes
There was a problem hiding this comment.
Deleting the barcode will cause all other codes to be shifted up by one.
As in it shifts them up into new rack locations? I think that would be bad if that's what you mean, as they are probably physically placing the tubes in a rack as they scan them.
There was a problem hiding this comment.
If you mean just shifted in colour index then that's acceptable I think. The colours are just a visual aid to see where the replicates are.
There was a problem hiding this comment.
Ah sorry, yes that comment was unclear...
Removing a barcode will cause all assigned colours to shift up by 1, the second explanation is correct.
There was a problem hiding this comment.
I noticed that functionality too, the colours changing when you remove a barcode. I thought it was ok.
app/javascript/multi-stamp-tubes/components/MultiStampTubes.spec.js
Outdated
Show resolved
Hide resolved
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1580 +/- ##
========================================
Coverage 90.65% 90.65%
========================================
Files 354 354
Lines 7273 7276 +3
========================================
+ Hits 6593 6596 +3
Misses 680 680
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
StephenHulme
requested review from
andrewsparkes,
KatyTaylor,
seenanair and
BenTopping
and removed request for
KatyTaylor and
seenanair
|
|
||
| const tube_machine_barcode = tube.labware.labware_barcode.machine_barcode | ||
| const tube_machine_barcodes = this.tubes | ||
| .filter((tube) => tube.state === 'valid') |
There was a problem hiding this comment.
TubeArraySummary checks against tube.labware.state === 'passed'. Without knowing the difference in the components I wanted to check that was correct.
There was a problem hiding this comment.
I must admit, I'm a little fuzzy on which states mean what. As far as I can tell, tube.state refers to whether the scanned in tube exists and is "valid", and tube.labware.state refers to the processing of a certain purpose (can be pending or failed)
| .map((tube) => tube.labware.labware_barcode.machine_barcode) | ||
|
|
||
| const barcode_index = findUniqueIndex(tube_machine_barcodes, tube_machine_barcode) | ||
| if (barcode_index !== -1) colour_index = barcode_index + 1 |
There was a problem hiding this comment.
Annoying that the css colours list doesn't start at 0.
There was a problem hiding this comment.
Yes.
I'm strongly considering refactoring the colours logic at some point soon.
app/javascript/multi-stamp-tubes/components/TubeArraySummary.vue
Outdated
Show resolved
Hide resolved
app/javascript/multi-stamp-tubes/components/MultiStampTubes.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I've had some trouble working out the data structure we're expecting for inputs and outputs of certain functions. Could we add docs, for example the following above the validTransfers function?
// validTransfers returns an array with the following structure:
//
// [
// { tubeObj: { index: 0, tube: {...} }, targetWell: 'A1' },
// { tubeObj: { index: 1, tube: {...} }, targetWell: 'A2' },
// ...etc...
// ]
//
There was a problem hiding this comment.
Good idea, started addressing in b4746cc, but more to come
There was a problem hiding this comment.
More added in 4ded3e7
There was a problem hiding this comment.
And in 826d23c
There was a problem hiding this comment.
Definitely helps with understanding what is going on in the component!
|
Code Climate has analyzed commit 826d23c and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 90.2% (0.0% change). View more on Code Climate. |
Closes #1427
Changes proposed in this pull request
Screenshots:
Deployment Notes
This should be tested against Integration Suite in UAT to confirm no breaking changes are introduced.
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to
main]- Check story numbers included
- Check for debug code
- Check version