[BUU] Stock level popout #11811

dacook · 2023-11-15T06:13:30Z

What? Why?

Working on [BUU] Change stock levels including on demand #11062

We're creating a new component as described in above issue. You can preview how it behaves here: https://www.figma.com/proto/v1zbrWDZSRd3Nqoe0SJ2Sm/Engineering-Delivery---Back-Office?page-id=708%3A5152&type=design&node-id=708-5153&viewport=278%2C123%2C0.33&t=HepYfjKLR82NcjTL-1&scaling=min-zoom&mode=design

Dev notes

I was able to keep the stimulus controllers nice and generic, and I hope to combine some existing controllers into one generic controller some day.

I wanted to make a "popout" ViewComponent with this, but remembered that

ViewComponent isn’t compatible with form_for helpers by default.

Still, it would be worth at least separating the CSS to a separate file.. 🤔
Or maybe it's worth trying to use ViewComponent and seeing if it happens to be possible.

What should Mario test?

I tried to make it behave how it seemed that it should. I covered these cases:

the button behaves like a select dropdown.
- A down arrow appears on hover
- If you select it with the keyboard,
  - 🆇 the arrow should appear but I couldn't get it working yet
  - you can press enter or the down arrow key to open it
- When you click outside of it, it closes
- the button shows the chosen value
'on demand'
- Selecting the checkbox closes it, and the button is focused
- De-selecting the checkbox enables the on_hand field, and focuses it for convenience
if any of the fields are changed, the button is marked changed
- 🆇 note that it doesn't yet properly handle the case where a changed field is disabled
- 🆇 also the changed checkbox isn't styled. I think it will need a css hack for that (which we might need anyway, because)
- 🆇 the checkbox style doesn't quite match the design (see below)

Checkbox style updates

Hrmm It doesn't quite match the design, but pretty close (not quite positioned right, and the border is not dark enough). It's possible to achieve the full design with a bit more effort, perhaps we can do that later (dev can see .redesigned-input as an example)
Some checkbox labels are now un-bold.

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

rioug

Very nice ! 👍 There was a couple of things I didn't understand, see my comment.

spec/javascripts/stimulus/bulk_form_controller_test.js

spec/javascripts/stimulus/popout_controller_test.js

dacook

Good questions! I will add a new commit.

spec/javascripts/stimulus/bulk_form_controller_test.js

spec/javascripts/stimulus/popout_controller_test.js

mkllnk

Looking good to me.

mkllnk · 2023-11-24T03:56:31Z

spec/javascripts/stimulus/popout_controller_test.js

+import { Application } from "stimulus";
+import popout_controller from "../../../app/webpacker/controllers/popout_controller";
+
+describe("PopoutController", () => {


I was wondering if we can solve this purely with CSS. And while that's possible, it wouldn't have the same functionality of reacting to down key events.

Example: https://codepen.io/imprakash/pen/GgNMXO

So cool.. thanks we should keep that in mind wherever a simple popup is needed.

HTML also has a dialog method, but funnily enough it requires javascript to make it work.
I wanted to use it but had a problem so had to revert to a div.

Interesting, I didn't know that one. A shame that they didn't include another way of opening a modal, for example with an href, button attribute or label. So the minimal code would be:

<a href="javascript:document.getElementById('modal').showModal();">Open</a>

Some very simple Javascript could probably bind that to an attribute like data-modal-open="modal" on any element. But it still needs Javascript while there are non-JS solutions out there.

By the way, I just had to try it out and was quite happy to have this minimal example working:

<html> <a href="javascript:document.getElementById('hello').showModal();">Show me</a> <dialog id="hello"> Hello! <form method="dialog"> <input type="submit" value="Nod"> </form> </dialog> </html>

app/webpacker/controllers/toggle_control_controller.js

It creates the false impression we can handle agregated stock.

But 'Array parameters do not play well with the check_box helper.' ...

The array format is generally fine, but to properly support checkboxes, we need this format with hash keys. https://guides.rubyonrails.org/form_helpers.html#understanding-parameter-naming-conventions

Also fixed it to line up properly. There's probably a better way to line it up but that's no my concern right now..

This doesn't _quite_ match the design, but would require a big CSS hack to customise it further, so I thought let's start with this.

Had to update the form controller a little bit to handle buttons. But arrow not showwing on focus. Getting some weird SCSS behaviour here.. maybe I'm trying to be too clever.

It looks like a select drop-down, so it can behave like one too.

I'm starting to think that these stimulus tests are worthless. The environment is not the same as a browser, which creates extra work to deal with the inconsistencies. And it means we're not testing real world behaviour. So these are just unit tests, but they take extra effort to put together due to the inter-relatedness with the DOM. Hmm.

Shoulda done this at the start.

I couldn't think of a simpler way to hardcode it, so now we have a clever generic method :) We can assume that hidden elements will stay hidden, but we need to check each time if an element is disabled or not.

This introduces a new 'toggle' controller, and we already had three\! So I created a generic interface that could be extended to potentially support all of them. I propose we try to reduce them all into the one controller, but won't go down the rabbit-hole just yet.. I have an idea on how to re-arrange and make it more contained, by assigning the controller only to the checkbox, and defining targets with aria-controls="", but chose to stick with Stimulus conventions for now.

Strangely, the spec passed when run locally, but not in CI. It's a curious case of the wrong letter case.

dacook · 2023-11-27T23:45:16Z

Rebased to resolve conflict.

I'm not sure if we want to do this on all fields, so just scoped it here for now.

dacook · 2023-11-29T04:37:23Z

Hi @mariocarabotta , I've updated the style of the popout disabled field: the value is hidden as per the design.

I've kept the style for other disabled fields the same for now. Let me know if you have a preferred style for those.

edit: sorry for the contradictory message, I've fixed it now

mariocarabotta · 2023-11-30T01:41:47Z

thanks @dacook just tested this and it's looking good!
re the disabled fields style: when I select on demand, does that field state become disabled?
does that mean that at the moment we have 2 types of styles for disabled fields?

I think we can push this through (at least from a visual point of view, will let testers do more testing).
just wondering if we should unify disabled styles at some point and how I should go about creating an issue for that

dacook · 2023-11-30T05:33:50Z

Hi @mariocarabotta, sorry about the delayed response.

Yes, I've implemented as disabled (and for the sake of your design, I made the value invisible). This is how the system currently works: we disable it but keep the old value stock level stored in the database (which does nothing).

We could instead clear the stock level, which arguably makes sense. This means if you later un-check 'on demand' you would not get your old stock level back. I'm not sure if this would matter.
In any case, we probably don't want to change the backend right now!

So yes, we have two styles of disabled fields, and I agree we should unify them. I wasn't sure if this design applied everywhere else, so I didn't apply it. I think it would be a quick change to apply it everywhere, let me know if you'd like to do that.
Otherwise, I'd suggest adding to the BUU backlog with a new design.

mariocarabotta · 2023-12-01T00:40:58Z

no need to change the backend.
I'll create a separate issue for unifying the disabled state, I'd like to do a bit of work on that eventually.

This issue can be tested and pushed from my side :) good work

RachL · 2023-12-01T10:19:17Z

Staging in FR failed twice https://openfoodnetwork.slack.com/archives/CEF14NU3V/p1701425061641669

RachL · 2023-12-01T15:12:42Z

I've checked the shopfront is updated accordingly - I did not test if if it is working with inventory, but I guess we can see that even after merge.

Aside of some weird overlap between on hand and producer column (which we can fix later) we are good to go :)

No hover:

With hover:

@dacook is the error on staging FR relevant? Otherwise I let you rebase and merge :)

mariocarabotta · 2023-12-04T00:57:38Z

@RachL we'll sort that one in a separate issue, already tracked

Aside of some weird overlap between on hand and producer column (which we can fix later) we are good to go :)

dacook force-pushed the buu-stock-levels-11062 branch 2 times, most recently from a901542 to ef3bc57 Compare November 22, 2023 06:10

dacook force-pushed the buu-stock-levels-11062 branch 2 times, most recently from 389bc8b to 1abedae Compare November 23, 2023 02:59

dacook marked this pull request as ready for review November 23, 2023 02:59

rioug approved these changes Nov 24, 2023

View reviewed changes

spec/javascripts/stimulus/bulk_form_controller_test.js Outdated Show resolved Hide resolved

spec/javascripts/stimulus/popout_controller_test.js Outdated Show resolved Hide resolved

dacook commented Nov 24, 2023

View reviewed changes

spec/javascripts/stimulus/bulk_form_controller_test.js Outdated Show resolved Hide resolved

spec/javascripts/stimulus/popout_controller_test.js Outdated Show resolved Hide resolved

dacook requested a review from rioug November 24, 2023 03:22

mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Nov 24, 2023

mkllnk approved these changes Nov 24, 2023

View reviewed changes

rioug approved these changes Nov 26, 2023

View reviewed changes

sigmundpetersen assigned dacook and mariocarabotta Nov 27, 2023

dacook added 16 commits November 28, 2023 10:43

Remove stock level from product row

4d22123

It creates the false impression we can handle agregated stock.

Add on_hand input

7aefa83

Add on_demand field

55cd8a6

But 'Array parameters do not play well with the check_box helper.' ...

Use form hash structure

eccfe96

The array format is generally fine, but to properly support checkboxes, we need this format with hash keys. https://guides.rubyonrails.org/form_helpers.html#understanding-parameter-naming-conventions

Move specific style rule to where it belongs

b7ac1f2

Also fixed it to line up properly. There's probably a better way to line it up but that's no my concern right now..

Update v3 checkbox style

d9570cd

This doesn't _quite_ match the design, but would require a big CSS hack to customise it further, so I thought let's start with this.

Style stock popout

d218565

[wip] Style on-hand button

735b578

Had to update the form controller a little bit to handle buttons. But arrow not showwing on focus. Getting some weird SCSS behaviour here.. maybe I'm trying to be too clever.

Show popout on click or down key

4560e37

It looks like a select drop-down, so it can behave like one too.

Hide popout when checkbox is checked

b604565

Refactor spec

888e0b9

Shoulda done this at the start.

Display summary of the popout values

9bc1e87

I couldn't think of a simpler way to hardcode it, so now we have a clever generic method :) We can assume that hidden elements will stay hidden, but we need to check each time if an element is disabled or not.

Count changed checkboxes

5e96368

Show changes on popout button

88fe8dc

Fix label

f034e46

Strangely, the spec passed when run locally, but not in CI. It's a curious case of the wrong letter case.

dacook force-pushed the buu-stock-levels-11062 branch from b683358 to f034e46 Compare November 27, 2023 23:44

mariocarabotta added the pr-staged-au staging.openfoodnetwork.org.au label Nov 28, 2023

dacook added 4 commits November 29, 2023 15:18

Refactor

aa79234

Update disabled field style

9ce511f

Hide value of disabled input in popout

ccc0c17

I'm not sure if we want to do this on all fields, so just scoped it here for now.

Fixup

6bf418c

dacook force-pushed the buu-stock-levels-11062 branch from b134488 to 6bf418c Compare November 29, 2023 04:36

mariocarabotta added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Nov 30, 2023

mariocarabotta removed the pr-staged-au staging.openfoodnetwork.org.au label Nov 30, 2023

mariocarabotta mentioned this pull request Dec 1, 2023

Unify text field disabled state #11883

Closed

RachL added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Dec 1, 2023

RachL added the pr-staged-uk staging.openfoodnetwork.org.uk label Dec 1, 2023

RachL added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Dec 1, 2023

dacook added technical changes only These pull requests do not contain user facing changes and are grouped in release notes and removed feedback-needed labels Dec 5, 2023

dacook merged commit 300998a into openfoodfoundation:master Dec 5, 2023
60 of 62 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUU] Stock level popout #11811

[BUU] Stock level popout #11811

dacook commented Nov 15, 2023 •

edited

Loading

rioug left a comment

dacook left a comment •

edited

Loading

mkllnk left a comment

mkllnk Nov 24, 2023

dacook Nov 24, 2023

mkllnk Nov 26, 2023

mkllnk Nov 26, 2023

dacook commented Nov 27, 2023

dacook commented Nov 29, 2023 •

edited

Loading

mariocarabotta commented Nov 30, 2023

dacook commented Nov 30, 2023 •

edited

Loading

mariocarabotta commented Dec 1, 2023 •

edited

Loading

RachL commented Dec 1, 2023

RachL commented Dec 1, 2023 •

edited

Loading

mariocarabotta commented Dec 4, 2023

[BUU] Stock level popout #11811

[BUU] Stock level popout #11811

Conversation

dacook commented Nov 15, 2023 • edited Loading

What? Why?

Dev notes

What should Mario test?

Checkbox style updates

Release notes

Dependencies

Documentation updates

rioug left a comment

Choose a reason for hiding this comment

dacook left a comment • edited Loading

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Nov 24, 2023

Choose a reason for hiding this comment

dacook Nov 24, 2023

Choose a reason for hiding this comment

mkllnk Nov 26, 2023

Choose a reason for hiding this comment

mkllnk Nov 26, 2023

Choose a reason for hiding this comment

dacook commented Nov 27, 2023

dacook commented Nov 29, 2023 • edited Loading

mariocarabotta commented Nov 30, 2023

dacook commented Nov 30, 2023 • edited Loading

mariocarabotta commented Dec 1, 2023 • edited Loading

RachL commented Dec 1, 2023

RachL commented Dec 1, 2023 • edited Loading

mariocarabotta commented Dec 4, 2023

dacook commented Nov 15, 2023 •

edited

Loading

dacook left a comment •

edited

Loading

dacook commented Nov 29, 2023 •

edited

Loading

dacook commented Nov 30, 2023 •

edited

Loading

mariocarabotta commented Dec 1, 2023 •

edited

Loading

RachL commented Dec 1, 2023 •

edited

Loading