-
Notifications
You must be signed in to change notification settings - Fork 13
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
Adding request context to scribble context #115
base: master
Are you sure you want to change the base?
Conversation
If I'm understanding this correctly, I can see how this would be useful but it makes the preview impossible since the preview view won't have the additional context provided by the original view. |
@mlavin How's the preview broken for you? All tests pass for me, and I haven't found any problems during my limited testing. |
I read this as you wanted to include all of the outer context from the original template into the scribble context. If that's the case, the preview view won't have that additional context. There won't be any breaking tests since we don't assert anywhere that this context isn't available and Django's template language will just render empty strings for the missing variables. If that isn't want you are proposing here then please clarify. |
"Create context for rendering a scribble or scribble preview." | ||
context = { | ||
context.update({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is modifying a mutable default argument. This has the potential to leak state between pages/requests/templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential alternative:
def build_scribble_context(scribble, extra_context=None):
"Create context for rendering a scribble or scribble preview."
context = {
'scribble': scribble,
}
if extra_context:
context.update(extra_context)
return context
@benred42 Yeah, already fixed that, but still tackling the addition of context to the preview. Will push an update later today probably. |
d188c5f
to
90e44c1
Compare
@mlavin @blablacio What is status of this PR? The fact, that Can I help somehow with this? |
Adding request context to scribble context. Useful for case like this:
When the form is submitted and there are form errors, they're not rendered as the request context is not passed to the scribble template. Not sure what side effects this might have.