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

Adding request context to scribble context #115

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blablacio
Copy link

Adding request context to scribble context. Useful for case like this:

{% load scribbler_tags %}
{% scribble 'login' %}
<form method="post">
  {% csrf_token %}
  <div class="form-group">
  {% for field in form %}
     {% if field.errors %}
     <div class="alert alert-danger">{{ field.errors|first }}</div>
     {% endif %}
  {% endfor %}
  {% for error in form.non_field_errors %}
    <div class="alert alert-danger">{{ error }}</div>
  {%  endfor %}
  </div>
  <div class="form-group">
    <input type="text" class="form-control" placeholder="Username" name="username">
  </div>
  <button type="submit" class="btn btn-primary center-block">Login</button>
</form>

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.

@mlavin
Copy link
Contributor

mlavin commented Nov 22, 2015

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.

@blablacio
Copy link
Author

@mlavin How's the preview broken for you? All tests pass for me, and I haven't found any problems during my limited testing.

@mlavin
Copy link
Contributor

mlavin commented Nov 23, 2015

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({
Copy link
Contributor

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.

Copy link
Contributor

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

@blablacio
Copy link
Author

@benred42 Yeah, already fixed that, but still tackling the addition of context to the preview. Will push an update later today probably.

@PetrDlouhy
Copy link
Contributor

@mlavin @blablacio What is status of this PR?

The fact, that django-scribbler doesn't support context variables is a huge limitation for me. It means that I can't use scribble on any part of my website that uses variables.
I don't care at all if it breaks the preview in case I use a variable. In such case the scribbler is now broken for me anyway.
It would be huge improvement if I could edit the template part at all.

Can I help somehow with this?

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