2596y25-595---display plate id on prepool plate schematic#2693
2596y25-595---display plate id on prepool plate schematic#2693
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2693 +/- ##
===========================================
- Coverage 84.59% 84.55% -0.04%
===========================================
Files 501 501
Lines 20538 20553 +15
Branches 377 377
===========================================
+ Hits 17374 17379 +5
- Misses 3161 3171 +10
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
It looks like we are already populating the plate barcode but it is just invisible as its black text on black background. See caption here https://github.com/sanger/limber/blob/develop/app/frontend/entrypoints/pages/multi_plate_pooling.js#L278 (line 278).
I think it would be best to add the input barcode in that caption too - should save you having to create those new html divs.
BenTopping
left a comment
There was a problem hiding this comment.
Changes look good to me - I have not tested locally.
Do we need tests for this?
BenTopping
left a comment
There was a problem hiding this comment.
Nice looks good to me.
…l-plate-schematic
…l-plate-schematic
…l-plate-schematic
StephenHulme
left a comment
There was a problem hiding this comment.
A couple comments - thanks for the screenshots, they look great!
| $('.plate-id-' + plateIndex + ' caption').html(` | ||
| <span class="barcode-label">${purpose}</span> | ||
| <span class="barcode-value">${barcode}</span> | ||
| <span class="caption-separator"></span> | ||
| <span class="barcode-label">${label}</span> | ||
| <span class="barcode-value">${inputBarcode}</span> | ||
| `) |
There was a problem hiding this comment.
There's potential for an injection attack here. Instead of using .html() with potentially user-supplied values, a better approach would be to specifiy the html tags directly and set their contents with .text() which would automatically escape any html.
Here is one (unchecked) suggested alternative:
const $caption = $('.plate-id-' + plateIndex + ' caption');
$caption.empty();
$caption
.append($('<span>').addClass('barcode-label').text(purpose))
.append($('<span>').addClass('barcode-value').text(barcode))
.append($('<span>').addClass('caption-separator'))
.append($('<span>').addClass('barcode-label').text(label))
.append($('<span>').addClass('barcode-value').text(inputBarcode));There was a problem hiding this comment.
Thanks for the comments. refactored.
| if (pipelineGroupName === 'ISC') { | ||
| label = 'Cherry Pick Plate ID' | ||
| } else { | ||
| label = 'Input Plate Barcode' | ||
| } |
There was a problem hiding this comment.
We tend to avoid pipeline-specific behaviour if at all possible as it can be unexpected and fragile if the pipeline name changes.
Is it reasonably possible to use a single label that applies to well to pipeline groups?
There was a problem hiding this comment.
refactored and moved the label to presenter


Closes #
Changes proposed in this pull request
update the template and javascript to display plate barcode & input barcode on top of source plate
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