Y24-382: Improve clarity of cherrypicking strategies and values #4544
Conversation
StephenHulme
changed the title
There was a problem hiding this comment.
Nice idea to refactor these out into separate files.
Since you're creating them from scratch, I would go for shorter names:
- cherrypick_by_concentration
- cherrypick_by_amount
- cherrypick_by_volume
Easier to read?
There was a problem hiding this comment.
Fixed in 208fd63
| @@ -0,0 +1,11 @@ | |||
| <div id='pick_by_nano_grams_per_micro_litre'> | |||
|
|
|||
| <%= render partial: 'form_group_radio', locals: { field_name: 'cherrypick', category: 'action', field_value: 'nano_grams_per_micro_litre', label_text: 'Pick by concentration (ng/µl)' } %> | |||
There was a problem hiding this comment.
I was a bit confused by field_name: 'cherrypick', category: 'action', field_value: 'nano_grams_per_micro_litre' so ended up reading the radio_button docs.
Wondering if your form_group_radio and form_group_text components provide much value, or just another layer of abstraction. I think they do - it does make this file easier to read, and saves you from repeating the css.
Maybe a comment somewhere to explain what the field_name, category and field_value are for would help 🤔
Also maybe use 'strategy' instead of 'action'?
| @@ -0,0 +1,9 @@ | |||
| <% options ||= {} %> | |||
| <% field_id = [field_name, category, field_value].join('_') %> | |||
There was a problem hiding this comment.
Wondering if there's a more Rails-y way of linking the label to the radio button, so that you don't have to form the id like this?
There was a problem hiding this comment.
Good point, eventually found a way, see f472384
There was a problem hiding this comment.
I was a bit confused as to what's going on with this one, because the existing file was not being used in the deleted app/views/workflows/_cherrypicking_volume_parameters.html.erb file.
This is modifying a component used in the fluidigm cherrypicking workflow - is this intentional, and tested?
There was a problem hiding this comment.
This is modifying a component used in the fluidigm cherrypicking workflow
This is intentional - but and now tested... (see below).
I realised that this shared common code with the primary cherrypick pipeline. Integrating it to use the same methods and templates as the primary cherrypick code was easier than trying to fix the bugs from different ways of doing the same thing.
Closes #4412
Changes proposed in this pull request
Before:
After:
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