Refactor Grant Summary implementation in Admin #3654
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3654 +/- ##
==========================================
+ Coverage 90.16% 90.18% +0.02%
==========================================
Files 254 254
Lines 6974 6969 -5
Branches 1028 1028
==========================================
- Hits 6288 6285 -3
+ Misses 610 609 -1
+ Partials 76 75 -1 |
estyxx
force-pushed
the
grants-recap-update
branch
from
baf1f75 to
fe0c80c
Compare
backend/grants/admin.py
Outdated
| statuses = Grant.Status.choices | ||
|
|
||
| # Apply filters to the Grant queryset | ||
| filter_params = request.GET.dict() |
There was a problem hiding this comment.
uhm this is probably fine because it is in admin, but I am not a big fan of just forwarding whatever is in the GET parameters to filter 🤔
There was a problem hiding this comment.
I moved this logic in _filter_and_format_grants to filter only by the allowed filters
| <tbody> | ||
| {% for key, counts in summary.items %} | ||
| {% with continent=key.0 country=key.1 %} | ||
| {% ifchanged continent %} |
| var checkboxes = document.querySelectorAll('.toggle-column'); | ||
| checkboxes.forEach(function(checkbox) { | ||
| checkbox.addEventListener('change', function() { | ||
| var countryColumn = document.getElementById('grantsByCountry').querySelectorAll('td:nth-child(' + checkbox.dataset.column + '),th:nth-child(' + checkbox.dataset.column + ')'); |
There was a problem hiding this comment.
why are you using both getElementById and querySelectorAll? can you not just use querySelectorAll?
There was a problem hiding this comment.
or maybe using JS template strings would look better
| cell.style.display = checkbox.checked ? '' : 'none'; | ||
| }); | ||
|
|
||
| var genderColumn = document.getElementById('grantsByGender').querySelectorAll('td:nth-child(' + checkbox.dataset.column + '),th:nth-child(' + checkbox.dataset.column + ')'); |
There was a problem hiding this comment.
using nth-child feels very hacky. Could we maybe use data attributes?
What are you trying to do here?
There was a problem hiding this comment.
Add the option to show and hide status columns
There was a problem hiding this comment.
I changed to use the data-column
backend/grants/admin.py
Outdated
|
|
||
| def _format_filters_for_display(self, filter_params): | ||
| """ | ||
| Formats raw filter keys to user-friendly names. |
There was a problem hiding this comment.
this comment doesn't look useful as it just explains what's the code after it? 🤔
There was a problem hiding this comment.
I changed this method and updated the description, let me know if now is more useful :)
| </table> | ||
| <script> | ||
| document.addEventListener('DOMContentLoaded', function() { | ||
| // This script allows users to toggle the visibility of columns in the table |
|
There are still various comments that explain what the code right below does, which I don't believe are helpful, but I will leave it to you to decide what you think makes sense to keep and what not. |
Why
This PR introduces a significant refactor of the
Grantsummary view in the Django admin panel. The previous implementation, which involved an auxiliary model GrantRecap and a convoluted override of the changelist_view, has been replaced with a cleaner, more maintainable, and extendable approach.How to test it