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

Pre-Election Reporting Dates Template #3227

Conversation

johnnyporkchops
Copy link
Contributor

@johnnyporkchops johnnyporkchops commented Oct 2, 2019

Summary (required)

This is the full-width page Wagtail template and accompanying JS that will allow us to achieve the mocked up experience and responsive design in issue #3016

Related PR: Closing #3188 in favor of this PR

Impacted areas of the application

modified: fec/static/scss/components/_table-styles.scss
modified: home/models.py
modified: ../fec/static/js/polyfills.js
modified: ../package-lock.json
modified: ../package.json
modified: fec/templates/base.html

new: fec/static/js/pages/reporting-dates-tables.js
new: home/migrations/0105_auto_20191002_1534.py
new: home/templates/home/full_width_page.html
new: home/migrations/0105_auto_20191002_1534

To Do: (All but the first one here are easy fixes)

  • Resolve issue with using accordion on this page. Accordion does not work for the All footnotes section. data-init.js (required for the A11Y dialog) interferes with the accordion trigger. I have left the native HTML5 Details element here for now.
  • Tweak any styling or design changes requested by @jonella
  • Clean up JS, review comments for brevity and clarity
  • Decide the cell-border color that works for both white and zebra-striped rows
  • switch hex colors in SCSS with proper variables (ie: #ddd becomes $gray-medium or whatever)
  • SCSS - Remove as many !important rules as possible - some may not be necessary

Screenshots

Screenshots to come soon

How to test

  1. Checkout feature/3016-reporting-dates-table-template
    Steps 2 through 6 are designed to keep your local cfdm_cms_test database in tact for working with other branches. You can forego this and just test this on feature.

  1. craetedb cfdm_cms_test_2019
  2. Get the latest database dump from Google drive
  3. pg_restore --dbname cfdm_cms_test_2019 --no-acl --no-owner <LOCAL PATH TO DUMP FILE NAME>
  4. export DATABASE_URL=postgresql://:@/cfdm_cms_test_2019 (see step 14 to point back)
  5. ./manage.py migrate

  1. CD to /fec-cms and rm -rf node_modules

  2. npm install

  3. npm run build

  4. cd to fec-cms/fec/ and ./manage.py runserver

  5. go to 127.0.0.1/admin

  6. create a page of type FullWidthPage

  7. create three html blocks and paste in the sections from the attached files into the three blocks. This step simulates the content team members copy/pasting from Excel but leaves out the actual spreadsheet part here.
    sample-data.txt

  8. When you are done, point your local database env var back to export cfdm_cms_test:
    export DATABASE_URL=postgresql://:@/cfdm_cms_test or if you use a bash profile with your database env var saved, just run:
    unset DATABASE_URL

@codecov-io
Copy link

codecov-io commented Oct 2, 2019

Codecov Report

Merging #3227 into develop will increase coverage by 0.01%.
The diff coverage is 87.5%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3227      +/-   ##
===========================================
+ Coverage     74.6%   74.62%   +0.01%     
===========================================
  Files          120      120              
  Lines         7160     7168       +8     
  Branches       633      633              
===========================================
+ Hits          5342     5349       +7     
- Misses        1818     1819       +1
Impacted Files Coverage Δ
fec/home/models.py 87.59% <87.5%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b7192d...843f3d7. Read the comment docs.

@JonellaCulmer JonellaCulmer self-requested a review October 3, 2019 17:31
{% endblock %}

<article class="main container">

Copy link
Member

Choose a reason for hiding this comment

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

Can you add the main title in here? <h1 class="heading--main">{{ self.title }}</h1>

border: none;
border-bottom: 1px solid $gray-light !important;
position: relative;
font-weight: bold;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed? It's making the whole row bold in mobile and I'm assuming we only want the .th_append to be bold in mobile?

Screen Shot 2019-10-03 at 9 56 34 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we only want the column names to be bold, not the dates.

@johnnyporkchops johnnyporkchops requested a review from rfultz October 4, 2019 12:55
Copy link
Contributor

@JonellaCulmer JonellaCulmer left a comment

Choose a reason for hiding this comment

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

I don't think we want the "All footers" section to be in an accordion. I'm not sure how it impacts how this page is printed, but that's a concern for Info. I don't mind having the footers all there in the event that some of our users find clicking on the numbers/symbols difficult to do for accessibility reasons. But I'm open to discussing it further. If we do proceed with an accordion, it has to be in our current style, which this is not.

@johnnyporkchops
Copy link
Contributor Author

I think we should consider two page types for full-width page.

  1. generic full-width page
  2. full-width-table page( which would start out as being just for the reporting dates tables ) and eventually become a template for any tables we want to use the new mobile view option.
    This way, if one wants to create a generic full width page without the dates tables, it will not include all of the javascript necessary to make the dates tables work.

@patphongs
Copy link
Member

I think we should consider two page types for full-width page.

I agree, we should have two in a separate PR.

Copy link
Member

@patphongs patphongs left a comment

Choose a reason for hiding this comment

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

@johnnyporkchops This looks great!

Copy link
Contributor

@JonellaCulmer JonellaCulmer left a comment

Choose a reason for hiding this comment

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

Everything looks great! Thanks, @johnnyporkchops

tr {
display: table-row ;
&.row_display {
display: table-row;
Copy link
Contributor

Choose a reason for hiding this comment

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

It isn't breaking anything but do we need this twice?

border: 1px solid $gray-light;

&:first-child {
border-left:none;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the linter say about spaces after colons? It only affects anything when we're searching the code looking for something specific, like "border: "—really minor thing.

}

tr.footnote_row {
/*background: $inverse !important;*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove the comments of unused code? (Or explain why we want them? (I know I add and then forget too many comments))

pfScriptElem.src = "{% asset_for_js 'polyfills.js' %}";
document.head.appendChild(pfScriptElem);
}
</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you! Sorry I forgot that one.

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.

Design a new table in Wagtail to display the 2020 congressional pre-primary reports info
5 participants