Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow to remove fields, filters and action from extended grid #264

Merged

Conversation

Florian-Merle
Copy link
Contributor

As of today, it's not possible to remove a field, a filter or an action from an extended grid.

For instance, in the case of Sylius customer_order grid in order to remove both the customer filter and the customer field, they are not removed but disabled.

This PR allows to solve this issue. For instance the customer_grid definition could be replaced by the following

sylius_grid:
    grids:
        sylius_admin_customer_order:
            extends: sylius_admin_order
            driver:
                options:
                    repository:
                        method: createByCustomerIdQueryBuilder
                        arguments:
                            customerId: $id
            sorting:
                number: desc
            removals:
                fields: ['customer']
                filters: ['customer']

Also fields, filters and actions from an extended grid can be removed using the GridBuilder.

@Florian-Merle Florian-Merle changed the title lfeat: Allow to remove fields, fitlers and action from extended grid feat: Allow to remove fields, fitlers and action from extended grid Sep 17, 2022
@vvasiloi
Copy link
Contributor

What's wrong with just disabling them?

@Florian-Merle
Copy link
Contributor Author

@vvasiloi disabling things when configuring a grid with yaml is not that bad I guess. The thing is, when configuring a grid in php with the GridBuilder, the removeField(...) and the other methods are misleading because they just don't work for things from the extended grid.

I mean, if we take a look at how Symfony form component works, we can remove forms from extended types with a dedicated method. IMO this should work in the same way and as I see it, disabling things is kind of a hack.

@Florian-Merle Florian-Merle force-pushed the feat/make-remove-work-for-extended-grids branch 3 times, most recently from 5b40c98 to 77d17fb Compare September 19, 2022 20:36
@loic425
Copy link
Member

loic425 commented Sep 21, 2022

That's really awesome. I think a PHPUnit test with an extended grid and a removed field is missing.

@loic425 loic425 changed the title feat: Allow to remove fields, fitlers and action from extended grid feat: Allow to remove fields, filters and action from extended grid Sep 21, 2022
@Florian-Merle Florian-Merle force-pushed the feat/make-remove-work-for-extended-grids branch 4 times, most recently from b2c9569 to db04d97 Compare September 21, 2022 17:36
@loic425
Copy link
Member

loic425 commented Sep 22, 2022

@Florian-Merle Dors that work with no grid inheritance, just updating an existing grid configuration

In a first file configuration

sylius_admin_grid:
    fields:
        name: 
            type: string

In a second file configuration

sylius_admin_grid:
    fields:
        removals: 
            fields: ['name']

@Florian-Merle Florian-Merle force-pushed the feat/make-remove-work-for-extended-grids branch from db04d97 to dcb4d54 Compare September 23, 2022 20:02
@Florian-Merle Florian-Merle force-pushed the feat/make-remove-work-for-extended-grids branch from dcb4d54 to 1e85607 Compare September 25, 2022 16:03
@Zales0123 Zales0123 added the Feature New feature proposals. label Oct 19, 2022
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, it's a nice feature 👍 I have one doubt that need to be resolved and IMO it's good to be merged 🎉

src/Bundle/Provider/ServiceGridProvider.php Outdated Show resolved Hide resolved
src/Component/Provider/ArrayGridProvider.php Outdated Show resolved Hide resolved
Copy link
Member

@lchrusciel lchrusciel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a little bit of mixed feelings and have the perspective, that if disabling on the inherited grids would work, that would be a cleaner and simpler solution, as we already have an implementation of it. My main problem with the introduction of field removal is that we will have two ways to achieve exact same thing. Which one should we recommend?

But I don't think, that this consideration should block us from merging it, as the proposed solution is good, solves a particular issue and does not interfere with our current design.

@Zales0123
Copy link
Member

Zales0123 commented Oct 19, 2022

I had also mixed feelings about it, but this comment convinced me it's a good way to go. I believe we can deprecate the disable option (as it indeed does the same as removals, at least in yaml) and remove it in potential 2.0. But still, this feature seems valuable.

One more thing, we do not have the documentation for that 💃 So it's another thing that can be done in a separated PR 🖖

Btw, I've allowed myself to fix the comment I left above, I will merge it when it's green 🚀

@Zales0123 Zales0123 merged commit e46b871 into Sylius:1.12 Oct 19, 2022
@Zales0123
Copy link
Member

Thanks, Florian! 🥇

@Florian-Merle Florian-Merle deleted the feat/make-remove-work-for-extended-grids branch October 19, 2022 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants