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

Improved templates to make easier to customize them for integration w… #478

Merged
merged 3 commits into from
Aug 31, 2022

Conversation

nad2000
Copy link
Contributor

@nad2000 nad2000 commented Jul 14, 2022

Fix for #477:

  • added block.super to reuse base block;
  • following admin convention added block "extrastyle" to facilitate customization;

Copy link
Collaborator

@marksweb marksweb left a comment

Choose a reason for hiding this comment

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

The call to super seems unnecessary as the block in base is empty so won't pull anything in?

@nad2000
Copy link
Contributor Author

nad2000 commented Jul 15, 2022

@marksweb it's done so that customization can be done in one place - the explorer/base.html. Otherwise, you would have to override all 4 templates: query_list.html, query.html, play.html and qyertlog_list.html.

Well here is my explorer/base.html that works with the proposed changes:

{% extends "explorer/base.html" %}

{% block extrastyle %}
  <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/5.13.0/css/all.min.css">
{% endblock %}

{% block sql_explorer_navlinks %}
  <li class="nav-item" id="admin:index">
    <a id="admin-link"
       class="nav-link"
       href="{% url 'admin:index' %}"
       data-toggle="tooltip"
       title="Research Funding Data Administration"
       >
       <i class="fas fa-radiation"></i> RFDA Admin</a>
  </li>
{% endblock %}


{% block sql_explorer_scripts %}
  <script>
    $(document).ready(function() {
      $('[data-toggle="tooltip"]').tooltip()
    });
  </script>
{% endblock %}

and this is how it gets rendered:
image

Without them, I would have to override almost the entire base.html.

@nad2000
Copy link
Contributor Author

nad2000 commented Jul 15, 2022

Actually, it was possible to refactor and dry a bit the templates and move the sql_explorer_navlinks block to the base.html.

Assuming that "django.template.context_processors.request" isn't used I populate the context in the mixin with the current view name.

@marksweb
Copy link
Collaborator

@nad2000 Ok now I get what you're doing so that makes sense 👍

I'll have a proper look at this over the weekend hopefully 🤞

@marksweb marksweb merged commit df49d4b into explorerhq:master Aug 31, 2022
@ernstki
Copy link

ernstki commented Jan 25, 2024

I'm overriding explorer/base.html in our application, just as @nad2000 prescribed above, but getting double the nav <li>s in the site's rendered output.

A possible workaround is to just copy-paste all the existing nav bar links, and add my two to the end, but it seems like Django's {{ block.super }} is supposed to take care of that.

I frankly didn't follow the context processor part of the conversation (beyond a vague understanding of what a context processor is), but could that have something to do with this? Or have I just done something boneheaded?

There is only this single file in ourapp/templates/explorer:

{# ourapp/templates/explorer/base.html #}
{% block sql_explorer_navlinks %}
{{ block.super }}
<li><a href="{{ DEFAULT_URL }}/explorer/schema/readonly">Schema</a></li>
<li><a href="#">|</a></li>                                              
<li><a href="{{ DEFAULT_URL }}/admin/">Back to {{ SITE_TITLE }}</a></li>
{% endblock %}

Screenshot of the effect described above

I'm currently using django-sql-explorer==3.2.1, the latest release as of this writing, and Django 3.2.latest. I get that this could actually be a Django bug, perhaps already fixed in some later version, and I am looking into that now.

Update: had nothing at all to do with this PR—my bad!

We did this, and the fix was this:

diff --git a/ourapp/settings.py b/ourapp/settings.py
index 15e8803..df73912 100644
--- a/ourapp/settings.py
+++ b/ourapp/settings.py
@@ -65,7 +65,9 @@ ROOT_URLCONF = "ourapp.urls"
 TEMPLATES = [
     {
         "BACKEND": "django.template.backends.django.DjangoTemplates",
-        "DIRS": [os.path.join(BASE_DIR, "ourapp/templates")],
+        # no need to specify additional 'DIRS': [] here; see
+        # https://docs.djangoproject.com/en/3.2/topics/templates/#configuration
+        # and https://stackoverflow.com/a/75021565/785213
         "APP_DIRS": True,
         "OPTIONS": {
             "context_processors": [

@nad2000
Copy link
Contributor Author

nad2000 commented Aug 30, 2024

The feature (the template block sql_explorer_navlinks) was removed with dc2b783 :(

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