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

Update email.rst #6264

Closed
wants to merge 1 commit into from
Closed

Update email.rst #6264

wants to merge 1 commit into from

Conversation

mikaelz
Copy link
Contributor

@mikaelz mikaelz commented Feb 11, 2016

Use name defined in the controller.

Use `name` defined in the controller.
@javiereguiluz
Copy link
Member

@mikaelz thanks for your contribution. However, in this case, I'm not sure about the proposed changes. It seems like You did it! You registered! is the heading of this email, so there is no need to add a new one before it.

@mikaelz
Copy link
Contributor Author

mikaelz commented Feb 12, 2016

IN the controller there is name variable defined and not used in the view.
It would be a good example how to pass variable from controller to view.

@xabbuh
Copy link
Member

xabbuh commented Feb 12, 2016

As this cookbook article focuses on sending e-mails, I would rather not pass the name variable to the template.

wouterj added a commit that referenced this pull request Jul 8, 2016
This PR was submitted for the master branch but it was merged into the 2.7 branch instead (closes #6264).

Discussion
----------

Update email.rst

Use `name` defined in the controller.

Commits
-------

847db7e Update email.rst
@wouterj
Copy link
Member

wouterj commented Jul 8, 2016

Hi @mikaelz!

I agree with you, if we show how to pass a variable, we should use it in the template as well. I think showing that you can pass variables to a template is quite important detail for some people.

I also agree with @javiereguiluz, that the h3 is the title of the email. That's why I slightly tweaked the template to use the variable below the h3 after merging your PR in c53d20f.

Thanks!

@wouterj wouterj closed this Jul 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants