-
Notifications
You must be signed in to change notification settings - Fork 40
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
Summary data tables #4088
Summary data tables #4088
Conversation
…for feature deploy
@johnnyporkchops Please see my thoughts based on previous screenshots you provided. Happy to discuss this with you further or talk through any potential problems or blockers to what I suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome—I couldn't break it! :)
Couple questions but I think it's good
StatisticalSummary.prototype.handlePushState = function() { | ||
///PUSH STATE 2//// | ||
const data = { year: this.chosenYear, segment: this.chosenSegment }; | ||
//const title = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to keep this?
let pressReleaseLinkClean; | ||
|
||
//Ugly but necessary conditionals for the inconsistent/incomplete press-release history | ||
if (this.chosenYear >= 2018 || this.chosenYear == 2014) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add notes anywhere else in the repo, linking this to other places that deal with the same issue? Would help us find places when/if the rules are no longer necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, this is the first time we have had to attempt to generate a link based on parameters
@@ -663,3 +663,43 @@ h3 + .simple-table { | |||
.breakdowns-home { | |||
width: 100%; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the linting rules trip on the indentations?
@@ -76,7 +76,7 @@ def _detect_space(repo, branch=None, yes=False): | |||
('dev', lambda _, branch: branch == 'develop'), | |||
# Uncomment below and adjust branch name to deploy desired feature branch to the feature space | |||
# ('feature', lambda _, branch: branch == '[BRANCH NAME]'), | |||
) | |||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the linter hate this?
Codecov Report
@@ Coverage Diff @@
## develop #4088 +/- ##
===========================================
- Coverage 75.47% 75.46% -0.02%
===========================================
Files 121 121
Lines 7412 7413 +1
Branches 596 596
===========================================
Hits 5594 5594
- Misses 1818 1819 +1
Continue to review full report at Codecov.
|
@JonellaCulmer: Style/text changes have been made and this is ready for final review. cc: @patphongs, @rfultz I can build the draft pages on production. Once this is deployed to prod, all we have to do is choose the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @johnnyporkchops, thank you for all your work on this!
} | ||
|
||
//Update state upon back or forward button click | ||
StatisticalSummary.prototype.handlePopState = function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job handling this!
if (this.chosenSegment > parseInt(latestAvailable)) { | ||
this.latest_segment_alert.textContent = | ||
latestAvailableOption.text + | ||
' is the latest available option for ' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like how you handled this for when someone tries to access an unavailable coverage period
@patphongs , @rfultz I added one more change to move the choices for Also, surprisingly, making this last change ( |
@johnnyporkchops I just finished the Party Summary page review and this is what I noticed:
I was able to build a page too by copying and pasting from the suggested page listed above. No problems. I am looking at the candidate summary page now. |
I finished going through the rest of the categories.
The pages look really clean and nice. I was able to create a draft page and copy blocks over to the new page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added my comments to the main ticket.
@johnnyporkchops is it okay for me to merge this? Thank you! |
Are we missing anything? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Ran it locally and it worked perfectly. Thanks, John!
Summary
selected attribute
on that segment select option in each Wagtail page. Any later time-period options will then be visibly disabled for the current year.Blocking: #3958
Related PR: #4018
Impacted areas of the application
new file: fec/static/js/pages/statistical-summary.js
new file: home/migrations/0110_auto_20200930_1419.py
modified: home/models.py
modified: home/templates/home/custom_page.html
modified: fec/static/scss/components/_table-styles.scss
modified: fec/fec/constants.py
reverted: tasks.py
Screenshots
Related PRs
WIP #4079
How to test
Content team
This is on feature:
https://fec-feature-cms.app.cloud.gov/campaign-finance-data/congressional-candidate-data-summary-tables
You can test building a Wagtail page using the instructions below.
Developers:
feature/3943-summary-data-tables
npm run build
./manage.py migrate
./manage.py runserver
http://localhost:8000/admin
-or-127.0.0.1:8000/admin/
CustomPage
statistical_summary.js
forConditional_JS dropdown
html blocks
using the sections provided below--OR-- use a page on feature as a model. Example: https://fec-feature-cms.app.cloud.gov/admin/pages/11032/edit/
Suggested tests:
statistical-summary.js
does not get included in other pages using CustomPage template.selected
attribute on the time-period select in Wagtail and confirm that later time-periods options become disabled for this year. If you are on another year with a later time-period selected, and switch back to this year, you should see an alert under the selects.Three html blocks for Wagtail CustomPage:
Selects
Table 1
Table 2