Skip to content

Feat: UI templates for the districts application #98

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

Merged
merged 6 commits into from
Apr 28, 2025
Merged

Conversation

lalver1
Copy link
Member

@lalver1 lalver1 commented Apr 22, 2025

Closes #68

This PR adds UI placeholders for the districts application.

Reviewing

Click around to test out the navigation sidebar to go from all districts to a particular district.

image

@lalver1 lalver1 self-assigned this Apr 22, 2025
@lalver1 lalver1 force-pushed the feat/districts-ui branch from 99698c3 to 450f24a Compare April 22, 2025 19:45
@lalver1 lalver1 changed the title Feat: UI templates for the districts application Feat: UI templates for the districts application Apr 22, 2025
Copy link

github-actions bot commented Apr 22, 2025

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  pems
  urls.py
  pems/core
  context_processors.py
  middleware.py
  urls.py
  pems/districts
  urls.py
  views.py
  pems/streamlit_sample
  urls.py
Project Total  

This report was generated by python-coverage-comment-action

@lalver1 lalver1 force-pushed the feat/districts-ui branch from 450f24a to 8285883 Compare April 22, 2025 19:59
@lalver1
Copy link
Member Author

lalver1 commented Apr 22, 2025

  • I'll have to review the naming/organization of the templates' context soon since it will grow quickly and it'll need to be better organized, but I think for the districts app this may work.
  • The borders on the form, chart, details and map divs will be removed, they will only be used temporarily to help us develop the layout.

@lalver1 lalver1 force-pushed the feat/districts-ui branch from 8285883 to 46f0969 Compare April 22, 2025 22:02
@lalver1 lalver1 marked this pull request as ready for review April 22, 2025 22:12
@lalver1 lalver1 requested a review from a team as a code owner April 22, 2025 22:12
@thekaveman
Copy link
Member

I'll have to review the naming/organization of the templates' context soon since it will grow quickly and it'll need to be better organized, but I think for the districts app this may work.

I've been thinking about this some as it relates to our context strategy and class-based views.

A couple of ideas I'm kicking around:

Hierarchical context

It is OK (and maybe preferable for organization) to have hierarchical context values using dicts e.g.

def get_context_data(self, **kwargs):
    context = super().get_context_data(**kwargs)

    districts_ctx = {}
    districts_ctx["all"] = District.objects.all()

    context["districts"] = districts_ctx
    return context

Then in the template:

<h1>Districts</h1>
{% for district in districts.all %}
   ...
{% endfor %}

Standard context

We might want to think about standardizing on some of the top-level keys. E.g. if we have a class-based view that adds context via a get_context_data() implementation, maybe we should always put that in the view top-level context?

class SomeView(TemplateView):
    def get_context_data(self, **kwargs):
        context = super().get_context_data(**kwargs)

        custom_ctx = {}
        custom_ctx["something"] = "data"  

        context["view"] = custom_ctx
        return context

This way we can easily distinguish between that from views and that from context processors in the template:

<h1>{{ view.title }}</h1>
<p>{{ view.content }}</p>
<p>{{ common.content }}</p>

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Left some comments.

I think it's time to look into more of the class-based views. A lot of the work here would come for free by using some more sophisticated views beyond TemplateView.

lalver1 added 3 commits April 23, 2025 18:24
context that will be shared across views of the districts app is
moved into the DistrictContextMixin. This reduces context duplication,
both when different views need the same context and also when the view
may not need the context, but the template it extends does need it.
Note that because of how the MRO and mixins work, DistrictContextMixin
inherits from TemplateView.
using DetailView for the DistrictView class simplifies the code and
improves readability in the districts/district.html template.
@lalver1
Copy link
Member Author

lalver1 commented Apr 23, 2025

Thanks for the suggestions @thekaveman!
I implemented a shared context (using your Hierarchical context idea) in 7b93b1f and switched to use a DetailView for DistrictView in 5cb85e2.

now the URL parameter is more descriptive, it is the district number,
and the template context refers to the currently selected district
@lalver1 lalver1 force-pushed the feat/districts-ui branch from 806efc8 to 6f2679c Compare April 28, 2025 19:52
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

👍

@lalver1 lalver1 merged commit 747489f into main Apr 28, 2025
9 checks passed
@lalver1 lalver1 deleted the feat/districts-ui branch April 28, 2025 21:07
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.

Create templates (UI) for the districts application
2 participants