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

[5.5] Add Route::view() helper #19835

Merged
merged 4 commits into from
Jul 2, 2017
Merged

[5.5] Add Route::view() helper #19835

merged 4 commits into from
Jul 2, 2017

Conversation

brayniverse
Copy link
Contributor

This adds a new Route::view() method. The implementation works with route caching as it uses an internal ViewController class.

Route::view('contact', 'pages.contact');

The first argument is the route and the second is the view name.

@brayniverse brayniverse changed the title Add Route::view() helper [5.5] Add Route::view() helper Jun 30, 2017
/**
* Create a new view controller instance.
*
* @param \Illuminate\Contracts\View\Factory $view
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing @return void

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

@lucasmichot
Copy link
Contributor

It would be more useful if we could also pass variables:
Route::view('contact', 'pages.contact', ['foo' => 'bar']);


$router->view('contact', 'pages.contact');

$this->assertEquals('Contact us', $router->dispatch(Request::create('contact', 'GET'))->getContent());
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess assertEquals is also fine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by this?

Copy link
Contributor

Choose a reason for hiding this comment

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

it compares the result AND the return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not clear whether you like or dislike my use of assertEquals here. Are you suggesting I should do something different?

Copy link
Contributor

@lucasmichot lucasmichot Jun 30, 2017

Choose a reason for hiding this comment

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

nevermind

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright. But I'm always happy to hear if I could have done something better so please do elaborate later if you change your mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess lucasmichot meant assertSame ;-)

@fitztrev
Copy link
Contributor

It would be more useful if we could also pass variables:
Route::view('contact', 'pages.contact', ['foo' => 'bar']);

What variables would you have in a route file? I think a view composer would be better if you need to do something like that.

@lucasmichot
Copy link
Contributor

Route::view('contact/germany', 'pages.contact', ['office' => 'Berlin'])->name('contact.germany');
Route::view('contact/england', 'pages.contact', ['office' => 'London'])->name('contact.england');

The goal of this PR is too make Route::view a fast shortcut to use, if you have to create a view composer to use it, because you cannot pass any variable, you lose a bit the benefit of it

@brayniverse
Copy link
Contributor Author

Interesting point, I hadn't considered that. If this works with route caching I don't see a problem with adding it. It would be a fairly simple change too.

@taylorotwell do you have any strong opinions on this?

@taylorotwell
Copy link
Member

Fine with me.

@joshmanders
Copy link
Contributor

Welp there goes my render helper I make for every projects! haha. Thanks for this PR @brayniverse

@taylorotwell
Copy link
Member

@brayniverse are you going to implement the data handling?

@brayniverse
Copy link
Contributor Author

Yes I will do. I've been driving all evening and the cars packed in. So I'll do it as soon as I get a chance but it probably won't be tonight.

@vlakoff
Copy link
Contributor

vlakoff commented Jun 30, 2017

@joshmanders That's the kind of helper probably a lot of people had added in their projects ;-)

@brayniverse
Copy link
Contributor Author

All should be good now. Data can be passed to the view and I tested it still works with route caching.

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

6 participants