Skip to content

Adds a few more blocks to base.html to make it more extendable #6337

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Adds a few more blocks to base.html to make it more extendable #6337

wants to merge 2 commits into from

Conversation

ababic
Copy link
Contributor

@ababic ababic commented Nov 27, 2018

Description

I currently have a need to output an additional form at the top of the page in the browsable API view. I'm happy creating a Renderer class with custom template to achieve this (extending api.html). But currently, my only option is to copy the entire content block from base.html in order to add something small. This is less than ideal.

Breaking the content block up into a few smaller blocks makes it easier to override just the relevant part of the template, with very few downsides.

With these changes in place, adding a custom request form to the template becomes as simple as:

{% block request_forms %}
    {{ block.super }}
    <div class="my-custom-form"></div>
{% endblock %}

Any/all feedback welcome :)

@tomchristie
Copy link
Member

Thanks. Perfectly reasonable, but I'd rather not introduce the churn here.

@ababic
Copy link
Contributor Author

ababic commented Nov 28, 2018

Hi @tomchristie,

Thanks. Perfectly reasonable, but I'd rather not introduce the churn here.

Sorry, does this mean you are in favour of the changes, but would rather implement them some other way, or that you would rather not have the template be changed at all?

Should I create an issue for this instead?

@tomchristie
Copy link
Member

Sorry, does this mean you are in favour of the changes, but would rather implement them some other way, or that you would rather not have the template be changed at all?

Probably the second of those two options.
(Although it'd be more feasible to consider a pull request that didn't have such a large change footprint, eg. leaving the indentation as-is, and just introducing the blocks. Not 100% sure where I would stand on that.)

@ababic ababic mentioned this pull request Nov 28, 2018
Closed
@ababic
Copy link
Contributor Author

ababic commented Nov 28, 2018

Okay @tomchristie, thanks for the feedback.

Sorry if I've made a bad impression here - it just seemed like a small, non-risky change that I could save everyone some time by implementing myself. I'm happy to revert the commit that added the indentation changes - However, that will leave the indentation in a bit of an inconsistent state (if only diffs had better detection/recognition of indentation changes!).

@tomchristie
Copy link
Member

tomchristie commented Nov 28, 2018 •
edited
Loading

It's not a bad impression at all, again I'd say perfectly reasonable proposal, it's just:

  • Not trivial to review the change.
  • Introduces extra override points that we're committing to keep supporting.
  • Introduces a little bit of template churn.

On balance I'm currently a "reasonable enough, but let's not". Might be a different balance on a PR that just introduced the blocks without any other changes, but I couldn't say for sure.

(Inconsistent indentation would also be a legitimate thing to tackle, separately, but we'd want a nice easy way of verifying that there weren't any other changes introduced except for whitespacing.)

@ababic
Copy link
Contributor Author

ababic commented Nov 28, 2018

Okay, thanks for confirming. I'll open a PR with just the block additions for now to make reviewing easier. If it's decided that those changes aren't acceptable on their own, then that's absolutely fine. I just thought these changes might help people in a similar situation.

@ababic ababic mentioned this pull request Nov 28, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ababic @tomchristie