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 params method to view macros #2736

Merged
merged 2 commits into from
Apr 21, 2021
Merged

Add params method to view macros #2736

merged 2 commits into from
Apr 21, 2021

Conversation

reziamini
Copy link
Contributor

In some cases we just need to pass some data like the title to the base layout and now to do that in Livewire we have to use layout method then pass the params as the second param like this :

return view('login')->layout('layouts.app', [
    'title' => 'Login Page'
]);

With these changes, we will be able to use a syntax like this:

return view('login')->params([
    'title' => 'Login Page'
]);

Also, all tests have been passed and new tests have been written!

@joshhanley
Copy link
Member

@rezaamini-ir nice PR, I definitely think this is something that would be useful!

The only thing I would consider changing is the name. To me reading view()->params() suggests they are parameters for the view. I think it needs to be clearer that they are layout parameters/attributes and not view ones.

There was an existing PR #2315 which did a similar thing but was closed.

@calebporzio the reason I think the original PR and this one would be of benefit is, so you don't need to specify the layout name each time, which you would if you are using the ->layout method and passing data to the layout as the second parameter as you suggested.

Especially if you are using default app layout as you don't need to use ->layout. So it would be nice to be able to pass data directly to the layout if need be.

Hope this helps!

@reziamini
Copy link
Contributor Author

@joshhanley Yes you are right about the name and use cases.
I think it's better to wait what does @calebporzio think about name and PR !

@zepfietje
Copy link
Contributor

I've also encountered this. What I found is that you can pass null as the view to the layout() method as a workaround.

That being said, I do feel like a dedicated method without having to specify the layout does make sense. In my own project I created a macro layoutData(), since it best describes what its purpose is. However, I feel like there could possibly be an even better name.

@reziamini
Copy link
Contributor Author

@zepfietje I'm agree with layoutData name for method

@calebporzio
Copy link
Collaborator

I could be down with this. Thanks for the input everyone.

I prefer layoutParams though to stay consistent.

Would you be able to PR this to the docs as well?

@zepfietje
Copy link
Contributor

@calebporzio wouldn't layoutData be more consistent with the Laravel core than layoutParams though?

@calebporzio
Copy link
Collaborator

Interesting, what are the references to Laravel core?

@zepfietje
Copy link
Contributor

zepfietje commented Apr 19, 2021

The Laravel docs mention passing data to views: https://laravel.com/docs/8.x/views#passing-data-to-views. I couldn't find any references where it's being referred to as params.

Also, internally it's named data instead of params:

@reziamini
Copy link
Contributor Author

reziamini commented Apr 19, 2021

@calebporzio So are agree to change the method name to layoutData?

Also, I will update docs for these changes if these changes are merged.

@calebporzio
Copy link
Collaborator

Yep! I dig!

@calebporzio calebporzio merged commit 75c47c9 into livewire:master Apr 21, 2021
@joshhanley
Copy link
Member

@calebporzio I think this hadn't been updated yet, so params() got merged instead of layoutData()

@reziamini
Copy link
Contributor Author

@joshhanley Yes you are right.
@calebporzio has merged params method

@calebporzio
Copy link
Collaborator

Ooops, adjusted now. Thanks!

@reziamini reziamini deleted the add_params_method branch April 22, 2021 13:57
@binaryweavers
Copy link

Not sure if changes are still being considered. I see that params implementation is not right and will override everything in
$this->livewireLayout. Like slot and section macro, shouldn't it be $this->livewireLayout['params'] = $params; ?
https://github.com/rezaamini-ir/livewire/blob/77e8b4b93d6da95dd219d4ade79dafb18dc49065/src/Macros/ViewMacros.php#L45

Second test is passing because it checks for main param set to foo but not for layout which is being set to default from config.
https://github.com/rezaamini-ir/livewire/blob/77e8b4b93d6da95dd219d4ade79dafb18dc49065/tests/Unit/ComponentLayoutTest.php#L82

@reziamini
Copy link
Contributor Author

Not sure if changes are still being considered. I see that params implementation is not right and will override everything in
$this->livewireLayout. Like slot and section macro, shouldn't it be $this->livewireLayout['params'] = $params; ?
https://github.com/rezaamini-ir/livewire/blob/77e8b4b93d6da95dd219d4ade79dafb18dc49065/src/Macros/ViewMacros.php#L45

Second test is passing because it checks for main param set to foo but not for layout which is being set to default from config.
https://github.com/rezaamini-ir/livewire/blob/77e8b4b93d6da95dd219d4ade79dafb18dc49065/tests/Unit/ComponentLayoutTest.php#L82

Thanks!
It was fixed in a new PR which has been mentioned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants