search

hyva-themes / magento2-hyva-admin

This module aims to make creating grids and forms in the Magento 2 adminhtml area joyful and fast.
https://hyva-themes.github.io/magento2-hyva-admin/
BSD 3-Clause "New" or "Revised" License
168 stars 39 forks source link

Grids for storefront #1

Open rav-redchamps opened 3 years ago

rav-redchamps commented 3 years ago

Hey @Vinai @wigman

This is not exactly an issue, it's a more of feedback or discussion.

How about renaming folder

vendor/hyva-themes/module-magento2-admin/view/adminhtml

to

vendor/hyva-themes/module-magento2-admin/view/base

That should allow the grid implementation in both storefront and admin.

For example it can be used to implement the "My Orders" section in customer account using Hyva grid.

I can sense that this is against the module name & purpose as it's built for backend but if we can use in both the areas then that sounds nice as well, at least to me :-) What say? I can submit a pull request for this change if you guys agree to the change.

I am planning to build the customer order grid implementation using Hyva grid as an open source module and having all the resources of parent module in base will be a easy and clean way else I have to write duplicate stuff in frontend view.

rav-redchamps commented 3 years ago

Hi again

Just realised that renaming folder "vendor/hyva-themes/module-magento2-admin/view/adminhtml" will not be enough, many changes are required.

For example,

Many more.

However, here is the proof of implementation of order grid

4af3d5fd-3b71-4299-963c-4a4ff1071daa

By doing above adjustments in the hyva-admin module.

Please let me know if you guys interested to open this up for frontend as well. I will prepare and submit the pull request.

Vinai commented 3 years ago

Oh my gawd, this is amazing! I love it 💕 Never thought of that use case.

Before merging a PR, I would like to discuss with @wigman about what that would mean in regards to css bundling size on the frontend. Also, maybe the alpinejs initialization can be improved as in the adminhtml it needs to coexist with requirejs. Maybe we need to split the core grid logic into a separate module that is used in the admin and the frontend slightly differently internally.

But in general, if using grids on the frontend is useful, heck yeah!

wigman commented 3 years ago

This is very cool indeed. There are a lot of complications that come with this though. There are factors we don't care about in admin that become a problem when it's facing customers in frontend. The tailwind bundle size being one of them (it's megabytes big).

It's entirely out of the scope for what this module is originally meant for and I personally don't have the resources available to work on that. It would also not work the same for Luma as it would for our own Hyvä frontend.

I'd recommend putting the phtml file in base and make the grid logic available to frontend, but separate the presentational layer into another module (and keep it in this repo for Hyva_Admin). With presentational layer I mean the way css is generated and alpinejs is being loaded.

In Hyvä Themes we'd have an entire different implementation of the 'presentational layer' because we already have alpinejs integrated and don't use requireJs so don't need a 'conflict-free' way to load it. We also have a tailwind generation mechanism for frontend there, so that would also not align with a "luma" integration.

I would even consider to also implement the grid.phtml file separately so that it uses Luma CSS and JS (just not knockoutJS). You have those resources already loaded after all. Adding alpineJS and Tailwind on top of that won't serve any customer any good to be honest. The real strength here is actually the generation of the data/table output.

mrtuvn commented 3 years ago

Imho i think only have some common thing for both grid area but keep separate for each area. Admin grid will never same as frontend grid. Not like Magento did previously hardcoded class grid used for both area https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Ui/view/base/ui_component/templates/listing/default.xhtml <= seem not good practices

Vinai commented 3 years ago

After mulling this over for a few days, my current thoughts are it would be easy to make the grid generation area independent, with basically the changes @rav-redchamps mentioned.

Before refactoring the module to make the grid generation logic area independent, I would like to improve the integration test suite a bit so it no longer depends on the sample data. I also would like to set up CI in github to make collaboration easier.. (I'll open issues for those tasks to track them separately: #6 and #5)

When the grid data generation is area independent we only would need frontend theme specific adapter modules @wigman mentioned, i.e. one for hyvä, one for luma etc. I'm happy @wigman bowed out of working on these since he has so much going on already.

I do not want a separate adminhtml-theme adapter module because of maintainability. It is a lot easier to develop and release the module when the adminhtml theme "adapter" is included.

Anyhow, I hope to have a bit of time for the prep tasks during the next week. Help is welcome, too.

Vinai commented 3 years ago

@mrtuvn I agree the ui components hardcoding admin classes in base area files is awful. sigh We will not repeat that mistake (I'm sure we will make plenty of mistakes of our own, but not that one).

Vinai commented 3 years ago

@rav-redchamps I've refactored out the grid dir locator logic and removed the hardcoded adminhtml in there.

Vinai commented 3 years ago

@rav-redchamps Also refactored the block logic into an abstract base class, with Block\Adminhtml\HyvaGrid andBlock\FrontendHyvaGrid extending it. See https://github.com/hyva-themes/magento2-hyva-admin/tree/main/Block

Also moved the di.xml and events.xml to global scope.

Vinai commented 3 years ago

@rav-redchamps And finally replaced the Backend\UrlInterface with the generic one. Thanks you very much for exploring what needed to be changed.

I haven't tested using the grid on the frontend, but I think the core logic should be in good shape now.

Vinai commented 3 years ago

What is still missing for frontend grids to be a thing?