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

Add Conditionable trait to support ->when() and ->unless() #5029

Closed
wants to merge 1 commit into from

Conversation

ziming
Copy link
Contributor

@ziming ziming commented Apr 13, 2023

WHY

BEFORE - What was wrong? What was happening before this PR?

Nothing

AFTER - What is happening after this PR?

The Conditionable trait from laravel 8 is added. (Backpack minimum laravel 8 version is higher than when this is added into laravel so it is very safe)

https://www.amitmerchant.com/conditionally-executing-callbacks-conditionable-trait-laravel-8x/

I think this will greatly improve the fluent flexibility and reduce if () clauses as you can chain methods on a conditional basis in a fluent way.

HOW

How did you achieve that, in technical terms?

Add the Conditionable trait

Is it a breaking change?

No

How can we test the before & after?

Try chaining ->when() or ->unless() method and inside it call other fluent methods conditionally

Feel free to edit this PR and add the Conditionable trait to other classes too (filters, columns.etc), I can add it too if you all want but tell me which classes

I think this is a small change that is a big improvement to developer DX

@tabacitu
Copy link
Member

I freaking LOVE this! Awesome idea @ziming .

@pxpm could you please take a look at it, this week or the next? After you free yourself of Dropzone, Uploaders, Media.

It's a really cool feature, I mean... this could be this months' 5.x feature, right?! Easy to do, significant impact.

My questions:

  1. Where should we use this? I'm thinking Fluent Fields, Fluent Columns, Fluent Filters; am I missing something?

  2. How do we document this?

Thanks @ziming !

@ziming
Copy link
Contributor Author

ziming commented Apr 13, 2023

Laravel's Macroable trait can be considered in another PR as well if it isn't done yet.

@tabacitu
Copy link
Member

Laravel's Macroable trait can be considered in another PR as well if it isn't done yet.

✅ Already done 😉

@maurohmartinez maurohmartinez requested a review from pxpm April 19, 2023 07:22
@maurohmartinez maurohmartinez removed the request for review from pxpm April 19, 2023 07:35
@maurohmartinez
Copy link
Member

Moving this here to continue it. Great idea @ziming... thanks!

@ziming ziming deleted the patch-6 branch July 5, 2023 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants