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

Ability to chain the root view #280

Merged
merged 1 commit into from
Jul 30, 2021
Merged

Conversation

MarGul
Copy link
Contributor

@MarGul MarGul commented Jun 24, 2021

Very small change and mostly syntax-sugar.

I have a guest section of my app where I need a new root-view.

Instead of doing

Inertia::setRootview('guest');

return Inertia::render('Guest/Home');

I can now do this instead: return Inertia::setRootView('guest')->render('Guest/Home');

Nothing much but I like it this way instead. What do you say?

@claudiodekker
Copy link
Member

claudiodekker commented Jul 28, 2021

Hey @MarGul,

Thanks for this! I do dig the idea, although the only thing I somewhat dislike is that:

  • setRootView doesn't read that nice when chaining
  • People rely on the current implementation of setRootView

Which makes me wonder whether we should (in addition to this change) add an 'alias' method that we could use for this? In either case, I'm definitely OK with these changes even if we don't find a great/better solution to that, so I'll just approve it for now as-is.

@reinink thoughts?

@claudiodekker claudiodekker requested a review from reinink July 28, 2021 08:38
@MarGul
Copy link
Contributor Author

MarGul commented Jul 28, 2021

@claudiodekker agree that the name setRootView doesn't read very well when chaining. Maybe we could add the rootview as a third param in render? Like Inertia::render('Gest\Home', ['param' => 'test'], 'guest')? Basically "just" make the render method call setRootView before returning the response? Maybe render is doing too much then but just an idea.

@reinink
Copy link
Member

reinink commented Jul 28, 2021

Hmm, my primary concern with this change is that Inertia::setRootView() currently affects things globally for Inertia. It will change the root view for all subsequent Inertia responses. I worry that chaining like this makes it feel like a local change only. For example:

$authResponse = Inertia::render('Auth/Home');
$guestResponse = Inertia::setRootView('guest')->render('Guest/Home');

if (Auth::user()) {
    return $authResponse; // will use "guest" root view
}

Obviously this is silly code, but it illustrates that someone could inadvertently change the root view globally without realizing it.

The better way to solve this is not in the ResponseFactory, but rather in the Response class. That way the root view change would just be applied to that response, and not changed globally. It also solves the naming issue.

return Inertia::render('Guest/Home')->rootView('guest');

Feel free to update this PR accordingly. 👍

Copy link
Member

@reinink reinink left a comment

Choose a reason for hiding this comment

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

Add rootView() to the Response class instead, as per this comment.

@MarGul
Copy link
Contributor Author

MarGul commented Jul 30, 2021

This was a way better solution @reinink ! I have pushed the code now and tried it out. Works perfect for me now because I have one rootView for my guest area and one rootView for my admin area that I easily can handle now. Thanks!

@reinink reinink merged commit 3a08d87 into inertiajs:master Jul 30, 2021
@reinink
Copy link
Member

reinink commented Jul 30, 2021

@MarGul Awesome, thanks!

@reinink
Copy link
Member

reinink commented Jul 30, 2021

This has been released: https://github.com/inertiajs/inertia-laravel/releases/tag/v0.4.4

(I still need to write the release notes)

@MarGul MarGul deleted the chainRootView branch July 30, 2021 12:16
@dillingham
Copy link

@reinink Maybe for 1.0 it could be called layout() throughout?

Nice feature for sure 🙌

@reinink
Copy link
Member

reinink commented Jul 30, 2021

@dillingham Hmm, not sure about that, since it's not a layout, it's a root view. I'd prefer to save the word "layout" for client-side layouts (https://inertiajs.com/pages#creating-layouts).

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.

4 participants