Introduce backoffice UI uplift #8996

jibees · 2022-03-14T20:57:48Z

What? Why?

Part of #8930

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

This one introduce the read-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 #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

mkllnk

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.

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.

mkllnk · 2022-03-28T00:49:18Z

app/constraints/new_products_page_constraint.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class NewProductsPageConstraint


I'm wondering if we can write a generic constraint factory because they are all the same except the feature toggle name.

app/views/admin/products/index.html.haml

app/webpacker/css/admin/all.scss

app/views/admin/products/index.html.haml

app/webpacker/controllers/hello_controller.js

app/components/product_component.rb

app/reflexes/application_reflex.rb

app/components/super_selector_component/super_selector_component.html.haml

app/components/search_input_component/search_input_component.html.haml

jibees · 2022-03-28T07:11:11Z

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 · 2022-03-28T09:37:13Z

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

mkllnk

Nice!

It's really cool to see it all working.

I tried to see the requests in the browser network console but they didn't show up. I guess that it's because it's using an open connection?

I reckon that this is good to be merged and then we can improve it. It's a lot of code. I hope that it will be less code than the old products page. 😉

app/components/products_table_component/products_table_component.html.haml

app/components/super_selector_component.rb

app/webpacker/controllers/super_selector_controller.js

app/components/products_table_component.rb

app/components/search_input_component/search_input_component.html.haml

app/components/super_selector_component/super_selector_component.html.haml

app/components/super_selector_component.rb

app/components/products_table_component.rb

seballot · 2022-04-03T16:07:00Z

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 · 2022-04-04T08:06:49Z

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 · 2022-04-04T20:20:48Z

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

jibees · 2022-04-05T06:24:16Z

Hi @jibees ! Ok I will first finish the report refactor #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 · 2022-04-05T06:47:05Z

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

Matt-Yorkley

There's a lot of complexity and coupling here and things being mixed together that maybe shouldn't be. I think we'd be better off if we start with the least amount of complexity first, and then add complexity where it's necessary and justified. I'd like to go in a different direction with this first PR, as it'll be foundational for the rest of the work. I'm thinking:

Remove the ViewComponentReflex library for now, I don't think we need it here
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
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
Are we writing multiple bespoke dropdowns components from scratch? Can we use a nice dropdown library instead?
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)

Matt-Yorkley · 2022-04-05T13:35:52Z

app/views/admin/products/index.html.haml

+= render partial: 'spree/admin/shared/product_sub_menu'
+
+#products_page
+  = render(ProductsTableComponent.new(user: spree_current_user))


The parts of the view layer wrapped up inside this component can just be separate parts; the filters, the table, and the pagination buttons.

A lot of the logic that this component holds can be pulled out and kept in separate places; the reflex methods can be in Reflexes and most of the other logic is for fetching the products from the database, which should live in the index action of the Admin::ProductsController.

Already done in "controllers"

(this key was added during the development of the PR)

Controller were instanced twice.

jibees · 2022-12-07T15:10:50Z

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

abdellani · 2022-12-10T11:28:30Z

app/reflexes/example_reflex.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+class ExampleReflex < ApplicationReflex


Can we remove this file?

I let that file because it contains a lot of documentation. But maybe we need to remove this.

I think @binarygit wanted to keep this as a reference, let's hear his opinion.
But maybe as we are getting more literate with Stimulus, we don't need this.

it contains alot of good information which is why I like keeping it around. I think it's okay to keep it, no? 😄

abdellani · 2022-12-10T11:47:07Z

app/components/products_table_component.rb

+      { label: I18n.t("admin.products_page.columns.#{column}"), value: column,
+        sortable: SORTABLE_COLUMNS.include?(column) }
+    }.sort! { |a, b| a[:label] <=> b[:label] }
+    @columns.unshift(ALL_COLUMN)


Please correct me if I'm wrong.
Looks like ALL_COLUMN only stored the name column. Maybe we can find a better name for it.

Right!

918827c

abdellani · 2022-12-10T12:01:42Z

app/components/product_component/product_component.html.haml

+        - if @image
+          .image
+          = image_tag @image.url(:mini)
+        = @name


This is looks good.

Just wondering if it'll be better to add a case for name

openfoodnetwork/app/components/product_component.rb

Lines 17 to 28 in 500c4e8

def column_value(column)

case column

when 'price'

@product.price

when 'unit'

"#{@product.unit_value} #{@product.variant_unit}"

when 'producer'

@product.supplier.name

when 'category'

@product.taxons.map(&:name).join(', ')

end

end

then you'll add one guard clause for the image, this will maybe require some CSS changes, something like:

%td.products_column{class: column[:id]} - if column[:id] == "name" && @image .image = image_tag @image.url(:mini) = column[:value]

Way simpler. Thanks! 🙏

0a19148

sigmundpetersen · 2022-12-14T08:19:38Z

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

RachL · 2022-12-14T09:45:41Z

@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 · 2022-12-14T13:19:07Z

@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:

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?

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

jibees · 2022-12-15T08:12:30Z

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.

jibees force-pushed the introduce-backoffice-ui-uplift branch from 35b3968 to c599c00 Compare March 15, 2022 15:13

jibees force-pushed the introduce-backoffice-ui-uplift branch from 08257d5 to a6fceba Compare March 25, 2022 13:26

jibees self-assigned this Mar 25, 2022

jibees marked this pull request as ready for review March 25, 2022 13:58

cillian mentioned this pull request Mar 25, 2022

Switch help modals from angular templates to use ViewComponent and StimulusJs #9040

Merged

mkllnk reviewed Mar 28, 2022

View reviewed changes

jibees force-pushed the introduce-backoffice-ui-uplift branch 4 times, most recently from 91f2b9b to d75697f Compare March 28, 2022 09:33

mkllnk approved these changes Mar 29, 2022

View reviewed changes

jibees force-pushed the introduce-backoffice-ui-uplift branch 5 times, most recently from d823455 to c32469e Compare March 29, 2022 12:21

mkllnk approved these changes Mar 29, 2022

View reviewed changes

app/components/super_selector_component.rb Outdated Show resolved Hide resolved

app/components/products_table_component.rb Show resolved Hide resolved

jibees force-pushed the introduce-backoffice-ui-uplift branch from a29902b to bc40fa9 Compare March 30, 2022 07:23

mkllnk approved these changes Mar 30, 2022

View reviewed changes

jibees force-pushed the introduce-backoffice-ui-uplift branch from bc40fa9 to ec1def2 Compare April 4, 2022 07:16

jibees force-pushed the introduce-backoffice-ui-uplift branch from ec1def2 to 08da016 Compare April 4, 2022 08:16

Matt-Yorkley reviewed Apr 5, 2022

View reviewed changes

jibees added 9 commits December 7, 2022 15:08

Click the whole Selector instead of arrow only

91d1ece

Toggle is now controlled by the browser (and don't requires a reflex)

940b554

Toggle is now controlled by the browser (and don't requires a reflex)

274ee2c

Handle filtering on js controller for SelectorWithFilter

3cf0162

Selectors are close by default

9dfc224

Do not import/initialize twice

ec31d63

Already done in "controllers"

As components: key already exists, use it!

f0e0bac

(this key was added during the development of the PR)

Remove white space

d97e9ae

Add missing i18n translation for PaginationComponent

5002870

jibees force-pushed the introduce-backoffice-ui-uplift branch from 866304b to 5002870 Compare December 7, 2022 14:09

Already handled by app/webpacker/controllers/index.js

500c4e8

Controller were instanced twice.

abdellani self-requested a review December 7, 2022 16:20

abdellani reviewed Dec 10, 2022

View reviewed changes

jibees added 2 commits December 12, 2022 15:20

Rename column "Name"

10bfe00

Simplify the way we render the name column

a827844

jibees force-pushed the introduce-backoffice-ui-uplift branch from 0a19148 to a827844 Compare December 12, 2022 14:20

abdellani approved these changes Dec 14, 2022

View reviewed changes

RachL self-assigned this Dec 14, 2022

RachL added the pr-staged-fr staging.coopcircuits.fr label Dec 14, 2022

RachL merged commit fd95970 into openfoodfoundation:master Dec 14, 2022

RachL removed the pr-staged-fr staging.coopcircuits.fr label Dec 14, 2022

This was referenced Jan 24, 2023

[BUU] As an enterprise user, I can display (read only) a list of products and filter them. #8930

Closed

[BUU] As an enterprise user, I can display (read only) a list of products and filter them -take II #10350

Closed

rioug mentioned this pull request Feb 7, 2023

Display missing columns #10394

Closed

mariocarabotta mentioned this pull request Apr 14, 2023

[BUU] See products in my catalogue #10694

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce backoffice UI uplift #8996

Introduce backoffice UI uplift #8996

jibees commented Mar 14, 2022 •

edited by sigmundpetersen

Loading

mkllnk left a comment

mkllnk Mar 28, 2022

jibees commented Mar 28, 2022

jibees commented Mar 28, 2022

mkllnk left a comment

seballot commented Apr 3, 2022

jibees commented Apr 4, 2022 •

edited

Loading

seballot commented Apr 4, 2022

jibees commented Apr 5, 2022

seballot commented Apr 5, 2022 •

edited

Loading

Matt-Yorkley left a comment •

edited

Loading

Matt-Yorkley Apr 5, 2022

jibees commented Dec 7, 2022

abdellani Dec 10, 2022

jibees Dec 12, 2022

sigmundpetersen Dec 12, 2022

binarygit Dec 14, 2022

abdellani Dec 10, 2022

jibees Dec 12, 2022 •

edited

Loading

abdellani Dec 10, 2022

jibees Dec 12, 2022 •

edited

Loading

sigmundpetersen commented Dec 14, 2022

RachL commented Dec 14, 2022

RachL commented Dec 14, 2022 •

edited

Loading

jibees commented Dec 15, 2022

		@@ -0,0 +1,11 @@
		# frozen_string_literal: true

		class NewProductsPageConstraint

		@@ -0,0 +1,38 @@
		# frozen_string_literal: true

		class ExampleReflex < ApplicationReflex

	def column_value(column)
	case column
	when 'price'
	@product.price
	when 'unit'
	"#{@product.unit_value} #{@product.variant_unit}"
	when 'producer'
	@product.supplier.name
	when 'category'
	@product.taxons.map(&:name).join(', ')
	end
	end

Introduce backoffice UI uplift #8996

Introduce backoffice UI uplift #8996

Conversation

jibees commented Mar 14, 2022 • edited by sigmundpetersen Loading

What? Why?

Some screenshots

What should we test?

Release notes

mkllnk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jibees commented Mar 28, 2022

jibees commented Mar 28, 2022

mkllnk left a comment

Choose a reason for hiding this comment

seballot commented Apr 3, 2022

jibees commented Apr 4, 2022 • edited Loading

seballot commented Apr 4, 2022

jibees commented Apr 5, 2022

seballot commented Apr 5, 2022 • edited Loading

Matt-Yorkley left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jibees commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jibees Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jibees Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

sigmundpetersen commented Dec 14, 2022

RachL commented Dec 14, 2022

RachL commented Dec 14, 2022 • edited Loading

jibees commented Dec 15, 2022

jibees commented Mar 14, 2022 •

edited by sigmundpetersen

Loading

jibees commented Apr 4, 2022 •

edited

Loading

seballot commented Apr 5, 2022 •

edited

Loading

Matt-Yorkley left a comment •

edited

Loading

jibees Dec 12, 2022 •

edited

Loading

jibees Dec 12, 2022 •

edited

Loading

RachL commented Dec 14, 2022 •

edited

Loading