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

Pass context to components through mount #1488

Conversation

matthewmcgarvey
Copy link
Member

Purpose

Related to #1152

Description

Instead of implementing mount_defaults the main code that we want passed down into components without being asked for is the context. This adds it to be passed in just like view is.

@stephendolan
Copy link
Member

@matthewmcgarvey This is awesome, and if we can extend @context to hold more than just HTTP::Server::Context (like current_site or current_user), then I think the need for mount_defaults goes away entirely!

I'm having trouble visualizing when I'd hit the scenario you describe here:

or set it with 'context(@context)' before rendering.

What would that look like in an app to render a component without mount?

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

That's funny, I had the same thought just the other day! Really, this is the big one. Anything else like current_user or current_site can still be as needed. This was really the only annoying one. I dig it!

@matthewmcgarvey
Copy link
Member Author

matthewmcgarvey commented May 6, 2021

On my phone so I can't link to the specific lines but at the bottom of this file where components can be provided also needs to pass in the context https://www.github.com/luckyframework/lucky/tree/master/src/lucky/renderable.cr

@stephendolan
Copy link
Member

Ah, right!

@matthewmcgarvey matthewmcgarvey marked this pull request as ready for review May 6, 2021 21:52
@matthewmcgarvey
Copy link
Member Author

@jwoertink @stephendolan You approved when it was a draft, is this good to merge now?

Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

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

Yup. Looks good to me

@stephendolan
Copy link
Member

Likewise! I don't see any downsides to getting this merged.

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.

3 participants