Skip to content

Conversation

@ysbaddaden
Copy link
Contributor

@ysbaddaden ysbaddaden commented Jan 24, 2023

Overall refactor of the React component and submitted params. BoxBatchesForm is roughly equivalent to BatchesSelector, but with an extracted BoxBatchForm sub-component and a somewhat simplified params, the component generates inputs to the form that is eventually sent by the browser (the forms are always rendered, but hidden with display:none).

No need to click the "ADD BATCH" or "ADD SAMPLE" buttons after choosing to create new samples or selecting. You can immediately go to select your first batch or sample.

No need to manually validate each concentration anymore: just enter valid values, otherwise the OK button is disabled. I removed the CANCEL button as the behavior was confusing to implement. it was currently remove batch, when it feels closer to undo (which didn't felt trivial to implement).

Capture d’écran de 2023-01-26 23-25-33@2x

The backend correctly verifies & reports errors for each purpose (verified with integration tests). On error, the form is re-populated. This works for both batches & samples. It should no longer be possible to create a box without samples, hence avoiding #1858. This is validated by integration tests.

Capture d’écran de 2023-01-26 23-26-51@2x

Last but not least, I added a "down chevron" to allow reopening a batch.

Capture d’écran de 2023-01-26 23-39-21@2x

closes #1845
closes #1846
closes #1859

Potential improvements:

  • mark the inputs as red with a tooltip when the replicate/concentration field is invalid (on input blur only).
  • validate the whole form and disable the SAVE button when something is invalid. Not easy because we're mixing vanilla JS with react components! Maybe we need a BoxForm component.

@ysbaddaden ysbaddaden self-assigned this Jan 24, 2023
@ysbaddaden
Copy link
Contributor Author

ysbaddaden commented Jan 26, 2023

I found another bug: the form is re-populated on error, but the batch UUID isn't properly set, so the batch is overlooked 😭 meh, smashed it 💪

I also broke some boxes controller test units. fixed typo

The component isn't generic but tied to the boxes#create action, and
goes much further than selecting a list of batches: for each batch we
select.

Extracts BoxBatchForm to handle the individual form for each form, with
one to many concentrations.

The React component now merely generates a plain form, never really
removing the actual inputs from the DOM, so they're always sent with the
form (no need for duplicated hidden inputs).

Last but not least, allows to reopen a batch form.
@ysbaddaden ysbaddaden force-pushed the refactor/improve-batch-selector-ux branch from 21c0017 to 11f7b6b Compare January 26, 2023 23:20
@ysbaddaden ysbaddaden changed the base branch from refactor/rename-samples-report-to-box-report to main January 26, 2023 23:21
@ysbaddaden ysbaddaden marked this pull request as ready for review January 26, 2023 23:23
@leandroradusky
Copy link
Contributor

Just brillant!!

I've pulled and tested and it's working neat.

The only important thing to add IMO, and I think this wasn't before the refactor Julien so it's not your fault, is the same validations for the samples creation than for the batches creation:

  • Variant boxes created from samples should contain samples coming at least from 2 different batches.
  • Challenge boxes created from samples should at least contain one distractor and one non-distractor sample.
  • LOD & other are good with what it's there.

I'm going through the code in detail and it looks great, let me take another look before approving ─the PR is quite long and some stuff is new to me, I'm learning while reading some cool stuff :)─

Copy link
Contributor

@leandroradusky leandroradusky left a comment

Choose a reason for hiding this comment

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

I went over the code and functionality and it looks good to me :)

@ysbaddaden ysbaddaden merged commit acd610d into main Jan 30, 2023
@ysbaddaden ysbaddaden deleted the refactor/improve-batch-selector-ux branch January 30, 2023 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

3 participants