Introduce backoffice UI uplift

[jibees]

What? Why?

Part of openfoodfoundation/openfoodnetwork#8930

As this PR is long enough, I do prefer divide it into two.

This one introduce the ready-only new page in the admin section, behind a feature toggle (the feature toggle is new_products_page): /admin/new_products. The missing part is to print a product row as it is specified in the issue openfoodfoundation/openfoodnetwork#8930

Some screenshots

What should we test?

This PR is more for a code review purpose than a real test, as the page is still not functional.

Release notes

Prepare the new Products page

Changelog Category: Technical changes

[jibees]

Exciting! Really cool work. Thank you.

🙏

But 66 commits for review? I think that it can be cleaned up a bit before a proper review. I looked through it all and left some random comments but it was too much to gain deep understanding, especially because some commits undo previous commits.

You're absolutely right, will do some git history cleaning. Back in dev

I wasn't sure with which goal you wanted this reviewed. I guess you've done a lot of work and wanted some affirmation that you are going in the right direction. I think you are.

I think that's it. I just wanted to have a small affirmation. Thanks! 🙏

[jibees]

I think I'm ready now. Much more clean, and only 35 commits.

[seballot]

Yeah, all of that with stimulus reflex and action cable, well done !!!

Few comments regarding UI

• The select box should open when we click anywhere in the input, currently it open only when clicking on the right arrow
• some elements does not fit quite with the design : missing grey background, table should have a large grey border-bottom, not a box-shadow, elements should have border-radius, floating labels should be smaller than input text and others, table header background should be darker and without border separation, pagination should be hidden when there is only one page, and probably some more :)

If you want a hand regarding polishing the design, I can help I like working on this kind of details :)

Great job, exciting !

[jibees]

Yeah, all of that with stimulus reflex and action cable, well done !!!

Thanks @seballot !

Few comments regarding UI

• The select box should open when we click anywhere in the input, currently it open only when clicking on the right arrow

Right, I'll make the changes needed. (done ✔️ )

• some elements does not fit quite with the design : missing grey background, table should have a large grey border-bottom, not a box-shadow, elements should have border-radius, floating labels should be smaller than input text and others, table header background should be darker and without border separation, pagination should be hidden when there is only one page, and probably some more :)

If you want a hand regarding polishing the design, I can help I like working on this kind of details :)

Oh ... 😊 I'd be happy if you could have a look... I can't refuse such a proposition! ~Would like to commit little as possible, I can then do some git history cleaning by squashing commits together after?~ (it seems to be difficult as files have been moved, ...)

Great job, exciting !

🙏 💪 🎉

[seballot]

Hi @jibees ! Ok I will first finish the report refactor openfoodfoundation/openfoodnetwork#9032 , then I'll try to participate in the UI uplift :)

[jibees]

Hi @jibees ! Ok I will first finish the report refactor openfoodfoundation/openfoodnetwork#9032 , then I'll try to participate in the UI uplift :)

Yes! Maybe we could open a new PR if this one is merged in the meantime.

[seballot]

Yes I think it would be the best, so I could directly work on openfoodfoundation/wishlist#255, applying the changes to both the old code and to your new code

[jibees]

Hi @Matt-Yorkley Thanks for reviewing it. Your comment is a bit difficult to answer, but will try. Just a few words on:

• Break up this mega-component (ProductsTableComponent) into smaller parts so they're not all nested and dependant on each other
• Keep everything as simple as possible and keep any components small and generic

That's exactly what I've been trying to do for components Selector, SelectorWithFilter, SearchInput, TableHeader and Pagination: they all have no relation with products or whatever, they are agnostic and they don't depend each others. I think they are small and generic enough, but happy to improve in someway. ProductsTable should be seen as an entire application/page but is coded as a component and is responsible of the whole state of this app (filters, selected, products, pagination, ...). This is exactly how I would have coded if I was in a React/component ecosystem.

• Define the code for the reflexes outside of the components, the only ones we're using here are simple and generic, which means they are potentially re-usable outside of these components

I'll give a try.

• There's quite a bit of UX behaviour being handled via reflexes which should just be handled by simple StimulusJS code on the client side instead, eg; opening and closing dropdowns shouldn't require triggering a reflex (which involves a call to the server)

To me, that's the main drawback of using this stack: we probably should manage UX state (dropdowns open or not, ...) and data state (products, selected filtering, ...) in different place (front and back). I'll give a try (at least to open/close dropdowns) : dbf515eee and 5ae47303a

Are we writing multiple bespoke dropdowns components from scratch? Can we use a nice dropdown library instead?

Not sure to understand what's the point here? is this relative to not reimplements some CSS/styling that have already been implemented? Would you like that I can give a try to tom-select introduced by https://github.com/openfoodfoundation/openfoodnetwork/pull/8814 ?

[jibees]

First issue i'm facing: open/close is managed in browser by adding/removing .selector-close class on a div (and closed on initialize()). This is straightforward and simple to manage.

  initialize() {
    this.close();
  }


  toggle = (event) => {
    event.preventDefault();
    this.element.querySelector(".selector").classList.toggle("selector-close");
  };

  close = () => {
    this.element.querySelector(".selector").classList.add("selector-close");
  };

But, when component is re-rendered due to a morph from its parent (ProductsTable when filtering), the component is re-rendered and don't know its state.

We don't actually have a unique source of truth, and I presume this kind of inconsistency should happen.

[jibees]

• Define the code for the reflexes outside of the components, the only ones we're using here are simple and generic, which means they are potentially re-usable outside of these components

Not sure what Reflexes you are thinking of, and not sure to see the plus-value extracting them. I see ViewComponentReflex as a nice helper to handle component morphing. What I've missed? Could you please point me something that I should extract as a standalone Reflex?

Here what I've started with Pagination:

diff --git a/app/components/pagination_component.rb b/app/components/pagination_component.rb
index af3664cbc..3b92a17bc 100644
--- a/app/components/pagination_component.rb
+++ b/app/components/pagination_component.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
-class PaginationComponent < ViewComponentReflex::Component
-  def initialize(pagy:, data:)
+class PaginationComponent < ViewComponent::Base
+  def initialize(pagy:)
     super
     @count = pagy.count
     @page = pagy.page
@@ -9,7 +9,6 @@ class PaginationComponent < ViewComponentReflex::Component
     @pages = pagy.pages
     @next = pagy.next
     @prev = pagy.prev
-    @data = data
     @series = pagy.series
   end
 end
diff --git a/app/components/pagination_component/pagination_component.html.haml b/app/components/pagination_component/pagination_component.html.haml
index 3f3620b2d..0d5e4318b 100644
--- a/app/components/pagination_component/pagination_component.html.haml
+++ b/app/components/pagination_component/pagination_component.html.haml
@@ -1,16 +1,16 @@
-= component_controller do
-  %nav{"aria-label": "pagination"}
-    .pagination
-      .pagination-prev{data: @prev.nil? ? nil : @data, "data-page": @prev, class: "#{'inactive' if @prev.nil?}"}
-        Previous
-      .pagination-pages
-        - @series.each do |page|
-          - if page == :gap
-            .pagination-gap
-              …
-          - else
-            .pagination-page{data: @data, "data-page": page, class: "#{'active' if page.to_i == @page}"}
-              = page
-      .pagination-next{data: @next.nil? ? nil : @data, "data-page": @next, class: "#{'inactive' if @next.nil?}"}
-        Next
+
+%nav{"aria-label": "pagination"}
+  .pagination
+    .pagination-prev{data: @prev.nil? ? nil : { reflex: "click->Pagination#change_page", page: @prev }, class: "#{'inactive' if @prev.nil?}"}
+      Previous
+    .pagination-pages
+      - @series.each do |page|
+        - if page == :gap
+          .pagination-gap
+            …
+        - else
+          .pagination-page{ data: { reflex: "click->Pagination#change_page", page: page }, class: "#{'active' if page.to_i == @page}"}
+            = page
+    .pagination-next{data: @next.nil? ? nil : { reflex: "click->Pagination#change_page", page: @next }, class: "#{'inactive' if @next.nil?}"}
+      Next
 
diff --git a/app/components/products_table_component/products_table_component.html.haml b/app/components/products_table_component/products_table_component.html.haml
index b01022763..63df32650 100644
--- a/app/components/products_table_component/products_table_component.html.haml
+++ b/app/components/products_table_component/products_table_component.html.haml
@@ -18,4 +18,4 @@
         = render(ProductComponent.with_collection(@products, columns: @columns))
 
   .products-table-form_pagination
-    = render(PaginationComponent.new(pagy: @pagy, data: reflex_data_attributes(:change_page)))
+    = render(PaginationComponent.new(pagy: @pagy ))

and app/reflexes/pagination_reflex.rb

# frozen_string_literal: true

class PaginationReflex < ApplicationReflex
  def change_page
    page = element.dataset['page'].to_i
    @page = page if page > 0
  end
end

[jibees]

@mkllnk

There's quite a bit of UX behaviour being handled via reflexes which should just be handled by simple StimulusJS code on the client side instead, eg; opening and closing dropdowns shouldn't require triggering a reflex (which involves a call to the server)

I agree with that specific point, and it's the most inconvenience I can see using that framework: we aren't encouraged to have a single source of truth. ~I'll implement it, and see how I can do this the best way I can.~ Already done actually, forgot that ;)

Are we writing multiple bespoke dropdowns components from scratch? Can we use a nice dropdown library instead?

Can't find any that was relevant. Actually, I'v re-created the almost same component in this recent PR: https://github.com/openfoodfoundation/openfoodnetwork/pull/9706

Break up this mega-component (ProductsTableComponent) into smaller parts so they're not all nested and dependant on each other

Actually I don't see any way of reducing it. It's basically a container that encapsulate smaller component.

Remove the ViewComponentReflex library for now, I don't think we need it here

As we already use ViewComponent and want to use stimulus_reflex, I thought it was a good idea to code as https://github.com/joshleblanc/view_component_reflex mentioned it. As far as I understood, it is used to include reflex inside component, which can be very handy (and the use of the helper reflex_data_attributes()

Keep everything as simple as possible and keep any components small and generic

I tried to. Any advices?

Define the code for the reflexes outside of the components, the only ones we're using here are simple and generic, which means they are potentially re-usable outside of these components

This is probably related to the previous point (ie. removing ViewComponentReflex). I'll have a look.

[jibees]

This one is probably (re, re) ready for review ; I'd be happy to have feedback from @binarygit @dacook or @abdellani

[sigmundpetersen]

Okay @jibees let's move this to Test Ready, no? 💪

[RachL]

@openfoodfoundation/testers I will take this one on as this is behind a feature toggle / the first of many PR before we reach a version ready to be tested in and out.

[RachL]

@jibees I will merge because the feature toggle is working fine and this PR is a monster :D . However I'm not sure what you meant by "the missing part is to print a product row". I see the following missing bits to close the issue:

• display variants and display the placeholder when there is no picture

current:

Previous:

• display missing columns : We can turn "import date" as "last update" like in the design but other than that I don't think we should loose columns?

Previous

current

• I see you've chosen to do all filter boxes the same size: it was easier i guess? On the design, sizes vary:

[jibees]

display variants and display the placeholder when there is no picture

That what I meant by "the missing part is to print a product row"

I don't think we should loose columns?

Seems like I forgot some of them right.

it was easier i guess?

Probably a miss too.