[Grid view] Display product as a grid#14345
Conversation
It makes product grid view feature flag available to angularJS
|
@mariocarabotta For the product overlay, I reused the template from the product preview in the backoffice, so it's slightly different. The main difference is the close button instead of the X in the corner we have currently. |
It renders a mostly empty turbo-frame for now. It's going to be used to wire the product grid view to the frontend
We don't need to load the product, as they will be displayed via the ProductsController. This is a bit hacky and we might still need to load products, time will tell.
The turbo-frame will load data automatically, and it will refresh when the order cycle is changed. It nicely leverage turbo-frame so we don't have to load data via "manually" adding AJAX request.
Which incidentally makes the spec better, we can now remove the use of `__send__`
We reuse the ProductsRenderer to keep the same loading logic. For experimentation purposes, we only load one page with 10 products.
I have tried leveraging modern CSS/Sass
It re uses the template created for the product preview, which is now in its own partial.
- Make it a bit rounded - Start at the start of the parent element, so it doesn't disappear when parent element next to the modal border. shift() middleware doesn't seem to work as expected
I am not exactly sure how to test that product don't load when the feature is enabled, but I don't want to waste time trying to figure it out.
got it! I will review better when it comes through to testing, for now it sounds good. I didn't realise you would have had to rebuild the overlay too. @chahmedejaz is also touching that part of the code for the multiple images carousel. Is that going to be an issue? |
|
I think we may have a bit of conflict, @mariocarabotta but it shouldn't be that serious. So we should be able to handle it. 👍🏻 |
mkllnk
left a comment
There was a problem hiding this comment.
Nice commit history. And it seems to be a good entry point to choose the product rendering layout on order cycle select. Just one question about caching below.
| @@ -1,5 +1,5 @@ | |||
| - # NOTE: make sure that any changes in this template are reflected in app/views/admin/products_v3/product_preview.turbo_stream.haml | |||
| = cache_with_locale do | |||
| = cache_with_locale(current_order_cycle&.id) do | |||
There was a problem hiding this comment.
Do we need to cache by feature toggle as well?
There was a problem hiding this comment.
we should, now that I think about it a bit more I am not sure the order cycle is needed but I'll double check that.
There was a problem hiding this comment.
I reviewed the cache key and made it feature aware, see: 24f3fbe
dacook
left a comment
There was a problem hiding this comment.
Great, thanks for your responses!
|
thanks, this is looking great already, very exciting!! some notes, feel free to pick up only what you think is reasonable for now, and we might address some of it later:
edit: adding this as a possible option to address this issue. Haven't looked into it too much but it seems to work with last row having fewer items
|
It allows 2 tiles till 360px wide.
DRY the css a bit, and make sure to use existing variable for colours.
dacook
left a comment
There was a problem hiding this comment.
Change requested as there's an unnecessary html attribute.
| %a{"data-tooltip-target": "element", href: @link, class: @link_class} | ||
| = @link_text | ||
| - if @no_link_element | ||
| %span{"data-tooltip-target": "element", href: @link, class: @link_class} |
There was a problem hiding this comment.
If it's not a link, then href isn't needed here and could be removed.
In fact, instead of adding a new parameter no_link_element, why not just branch if @link.present?
There was a problem hiding this comment.
Good catch !
There are many places where the tooltip doesn't have a link ie :
We could probably review that but I think that's out of scope of this PR.
There was a problem hiding this comment.
So there's other tooltips that have an <a href="">? That seems like the same case that you are implementing here, so think it would be better to clean up now and unify with the existing tooltips instead of adding a parameter and code branch for a new case.
Or if we don't clean up the old ones, can this new tooltip work with the existing <a href=""> form?
Sorry to be a pain but I think it's worth avoiding too many switches on a component, because we end up with lots of different usages and more room for bugs to hide, or visual inconsistencies.
There was a problem hiding this comment.
If I remember correctly, this PR makes the tooltip component available on the frontend. That's great, and in that case it would be a great opportunity to also remove "admin" from the name, which would make it nice and succinct.
There was a problem hiding this comment.
I was trying to not got there, but I guess that was inevitable once I had to actually make change to the component. Anyway, it's fixed now. it starts here : ab3e1ac
Do not use a link tag if no link provided
Replace old angular tooltip by the new stimulus based one.
And fix the styling for product preview
Update all "What's this ?" by the new component on the admin enterprise form.
dacook
left a comment
There was a problem hiding this comment.
Cool, thanks for continuing the clean up, looks like a good end result! 👍




Roadmap 2026: #84 - Grid viewwhen working on thisWhat? Why?
First step of Grid view, this PR show a basic grid view limited to one page and 10 products max. There are now variant information for now. This PR also include some refactor around tooltip, it makes the TooltipComponent available to the frontend.
What should we test?
Make sure to enable the
product_grid_viewfeatureWith a shop with one order cycle open:
-> products should be displayed as a grid
-> product overlay should display product information
-> Tool tip shows
With a shop with multiple order cycle open:
-> products should be displayed as a grid
-> product list should update and products should be displayed as a grid
Try with different grid size, mobile examples :
Tooltip
This PR includes some changes around tooltip :
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.