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

Add Congresses to model admin and implement footnotes #184

Merged
merged 4 commits into from
Apr 27, 2020
Merged

Conversation

beamalsky
Copy link
Contributor

@beamalsky beamalsky commented Apr 23, 2020

Overview

Closes #178. This PR adds the ability to create footnotes for each Congress on the site, and adds them to the templates following Jamie's request below. Also, while adding Congress to the ModelAdmin, I've added a more user-friendly way to adjust the inactive_days value for each Congress (as discussed in #176)

From Jamie, here are the 3 places footnotes should show up:

  1. Next to the congress on individual committee pages (footnote should be placed directly under chart)
    unnamed

  2. Next to the “Projected Grade” on individual committee pages (footnote should be placed directly under chart)
    unnamed (1)

  3. Next to Current Congress (projected) on landing page (footnote should be at bottom of page)
    unnamed (2)

Testing Instructions

  • If you haven't recently, download an updated db from production here. Add it to the root of your project as hearings.dump
  • docker-compose -f docker-compose.yml -f docker-compose.db-ops.yml run -e PGPASSWORD=postgres --rm dbload-dump
  • docker-compose run --rm app python manage.py migrate
  • docker-compose up
  • Log into Wagtail and go to the "Edit Congresses" section of the left sidebar. Add one and then multiple footnote values and make sure they shows up on the site as described above

@beamalsky beamalsky changed the title [WIP] Add Congresses to model admin and implement footnotes Add Congresses to model admin and implement footnotes Apr 23, 2020
@beamalsky beamalsky requested a review from hancush April 23, 2020 19:47
Copy link
Member

@hancush hancush left a comment

Choose a reason for hiding this comment

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

This seems fine, if a little unconventional! I don't love a non-standard way of show the asterisks, but I also understand each place you need to show it is slightly different. One thing that sticks out: Is there any need to show footnotes on the by-congress comparison view? Seems pretty important to note!


class CongressAdmin(ModelAdmin):
model = Congress
menu_label = 'Edit Congresses' # ditch this to use verbose_name_plural from model
Copy link
Member

Choose a reason for hiding this comment

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

Do we need these comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope!


count = 1
for rating in committeeratings:
if rating.congress.has_footnote:
Copy link
Member

Choose a reason for hiding this comment

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

Clever. This feels a bit unconventional, but I'm having trouble thinking of an alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I also spent a while trying to think of something that's more built into Django, but this is where I landed. If you think of another way later I'd be curious to hear it! I think this is ok for one instance on the site, but if we had this same problem for other types of data it would be worth abstracting somehow.

@@ -297,6 +321,13 @@ def percent_passed(self):

return cap_100(percent_passed)

@property
Copy link
Member

Choose a reason for hiding this comment

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

Does this offer any advantage over if congress.footnote?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point 🤦🏻‍♀️

@beamalsky
Copy link
Contributor Author

Thanks @hancush! I added footnotes to the Congress comparison page like you suggested—though Jamie didn't specifically request it, I think you're right that consistency necessitates it.

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.

Set up a way to add footnotes to Congresses
2 participants