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 fluent field group #2951

Merged
merged 21 commits into from
Jul 22, 2024
Merged

Add fluent field group #2951

merged 21 commits into from
Jul 22, 2024

Conversation

pxpm
Copy link
Contributor

@pxpm pxpm commented Jun 18, 2020

REFS: #2642

Address #2950

 CRUD::fieldGroup(
            CRUD::field('backpack')->label('backpacker'),
            CRUD::field('backpacker')->label('backpack')
        )->tab('Backpack Rocks')->label('Backpack Label Override Rocks.');

@tabacitu
Copy link
Member

Just. Freaking. Awesome. Great work @pxpm ! This is the kind PR I love - surgical. Good job!
Totally agree we need this - I will test and give feedback as soon as you let me know it's ready.

Cheers!

@pxpm
Copy link
Contributor Author

pxpm commented Jun 19, 2020

It's ok now @tabacitu you can test and merge.

Best,
Pedro

@lotarbo
Copy link
Contributor

lotarbo commented Jun 19, 2020

@pxpm if using this code, removing $field->save();

public function __call($method, $parameter)
    {
        foreach ($this->fields as $field) {
            $field->{$method}($parameter[0]);
        }

        return $this;
    }

its working with nested group)
Like this

CRUD::fieldGroup(
            CRUD::fieldGroup(
                CRUD::field('title'),
                CRUD::field('descr')
            )->size(6),
            CRUD::fieldGroup(
                CRUD::field('word'),
                CRUD::field('char')
            )->size(8)
        )->tab('Test')
        ;

@pxpm
Copy link
Contributor Author

pxpm commented Jun 19, 2020

Indeed, calling the method directly allow us to remove the save call.

Thanks

@lotarbo
Copy link
Contributor

lotarbo commented Jun 19, 2020

@pxpm @tabacitu also, group works with colum) So...change method name to group?)
Nice, pretty sugar CRUD::group();

@pxpm
Copy link
Contributor Author

pxpm commented Jun 19, 2020

@pxpm @tabacitu also, group works with colum) So...change method name to group?)
Nice, pretty sugar CRUD::group();

I don't dislike it, if @tabacitu agree I am ok with the change to make it seamless to work with fields and columns. Or create a columnGroup() helper in case group() is too vague.

@lotarbo
Copy link
Contributor

lotarbo commented Jun 19, 2020

@pxpm @tabacitu also, group works with colum) So...change method name to group?)
Nice, pretty sugar CRUD::group();

I don't dislike it, if @tabacitu agree I am ok with the change to make it seamless to work with fields and columns. Or create a columnGroup() helper in case group() is too vague.

But it works with column atm

CRUD::fieldGroup(
            CRUD::column('name'),
            CRUD::column('order')
        )->type('text');

@tabacitu
Copy link
Member

Excellent idea with group @lotarbo - I think it's intuitive because it's similar to Route groups in Laravel. So it'd be:

CRUD::group(
            CRUD::column('name'),
            CRUD::column('order')
        )->type('text');


CRUD::group(
            CRUD::field('name'),
            CRUD::field('order')
        )->type('text');

I think that's a great idea!

@tabacitu
Copy link
Member

What do you guys think of also supporting an array as parameter? (it's do the exact same thing)

It looks to me like it's easy to get confused what the parameter(s) for the group method should be, so it might be a good idea to support both.

CRUD::group(
            CRUD::column('name'),
            CRUD::column('order')
        )->type('text');

// or

CRUD::group([
            CRUD::column('name'),
            CRUD::column('order'),
        ])->type('text');

Not sure about this one - asking for opinion.

Personally I'd probably go with arrays in my projects because I like the fact that I can end all lines with a trailing comma. Though I believe it also works in parameters for PHP 7.3+ I believe... So that would render that argument mute 😆

@pxpm
Copy link
Contributor Author

pxpm commented Jun 19, 2020

I think you are right about the 7.3 trailling commas. But if you think we should support arrays for < 7.3 i am ok with it, but i personally would not be using the array notation myself.

@lotarbo
Copy link
Contributor

lotarbo commented Jun 19, 2020

trailing comma works since php7.3, and i think, no need support array

src/app/Library/CrudPanel/ObjectGroup.php Outdated Show resolved Hide resolved
src/app/Library/CrudPanel/ObjectGroup.php Outdated Show resolved Hide resolved
@pxpm
Copy link
Contributor Author

pxpm commented Jun 23, 2020

Added support for array input and refactored name to CrudObjectGroup

@lotarbo
Copy link
Contributor

lotarbo commented Sep 4, 2020

@pxpm @tabacitu any news with this?)

@pxpm
Copy link
Contributor Author

pxpm commented Sep 4, 2020

Hello @lotarbo

This is pending review. We had some major bugs to work out first and freezed the introduction of features. I think it's stabilizing now and we should go back to features soon.

Did you tried this PR? It's working good? Give us some feedback it might help speed up the review process.

Best to you,
Pedro

@tabacitu tabacitu changed the title [4.2] Add fluent field group Add fluent field group May 3, 2022
@tabacitu tabacitu changed the base branch from master to main July 31, 2022 05:34
@dwedaz
Copy link

dwedaz commented Sep 3, 2023

hi, any progress in v6?

@pxpm pxpm force-pushed the add-fluent-field-group branch from 3181af1 to 5d42726 Compare July 22, 2024 09:03
@pxpm pxpm merged commit cb95ac0 into main Jul 22, 2024
8 checks passed
@pxpm pxpm deleted the add-fluent-field-group branch July 22, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants