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

Feature/round of frontend cleanup #1641

Merged
merged 17 commits into from
Dec 15, 2017
Merged

Conversation

xtine
Copy link
Contributor

@xtine xtine commented Dec 11, 2017

Summary

This is a round of front end cleanup/refactoring on JavaScript initialization files and modules.

  • Remove old references to intro tour feature
    • Remove old tour__point classes that contained tour tooltip content and TOUR_PAGE JS global variables
  • Remove references to cms_url and webAppUrl now that we can directly reference relative paths
  • Remove extraneous code in site nav
  • Remove extra typeahead initialization in Django base template
  • Remove StickyBar module that is not being used anymore (was previously used on election page before redesign)
  • Hide glossary from being revealed when hidden (trying to overscroll)
  • Remove unused between-commitees.jinja template. https://github.com/18F/fec-cms/issues/1632
  • Renamed landing.js to data-landing.js to be more specific that it is JS for the landing page of the data section.
  • Fix glossary close page jump bug: https://github.com/18F/fec-cms/issues/1407
  • Add documentation to not-in-use JS module reaction-box.js

Impacted areas of the application

List general components of the application that this PR will affect:

  • Removes stale code for site nav and typeahead, will need to double check that this does not cause any regression errors.
  • General scrolling on site to prevent overscrolling.

Screenshots

Fixes this overscrolling issue:
screen shot 2017-12-11 at 3 04 56 pm

@codecov-io
Copy link

codecov-io commented Dec 11, 2017

Codecov Report

Merging #1641 into develop will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1641      +/-   ##
===========================================
- Coverage     79.4%   79.33%   -0.08%     
===========================================
  Files           46       45       -1     
  Lines         3297     3271      -26     
  Branches       497      493       -4     
===========================================
- Hits          2618     2595      -23     
+ Misses         679      676       -3
Impacted Files Coverage Δ
fec/fec/static/js/modules/feedback.js 97.26% <ø> (-0.04%) ⬇️
fec/fec/static/js/modules/site-nav.js 95.23% <100%> (-0.51%) ⬇️

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 01ed89d...80a3b23. Read the comment docs.

Copy link
Contributor

@jenniferthibault jenniferthibault left a comment

Choose a reason for hiding this comment

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

Ran this locally and spotted two unexpected things. Though they could be due to an error in my setup, so would appreciate help double-checking.

First, on the /data section, there's a DEVELOPMENT banner that's not appearing for other sections of the site.

screen shot 2017-12-12 at 7 54 14 pm

Second the raising/spending overview data visualizations disappeared. This may be due to an error in running my local env, but I'm suspicious of that because the API seems to be connected and populating everything else just fine.

screen shot 2017-12-12 at 7 55 04 pm

@@ -327,7 +327,5 @@
{% block scripts %}
<script src="{{ asset_for_js('dataviz-common.js') }}"></script>
<script src="{{ asset_for_js('landing.js') }}"></script>
<script>
TOUR_PAGE = 'campaign-finance-data';
</script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this closing <script> supposed to get deleted too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oof, good catch! Should have removed closing tag too.

@xtine
Copy link
Contributor Author

xtine commented Dec 13, 2017

@jenniferthibault:

  • fixed election map and line charts not showing up, it was because I tweaked how scrolling worked (to prevent overscrolling).
  • apparently that dev banner has only just shown up on data (previously openfec-web-app), and it was still linking to beta.fec.gov so I just removed it entirely.

@lbeaufort lbeaufort self-requested a review December 13, 2017 17:02
@ccostino
Copy link
Contributor

I tested this locally as well and came across a couple of things so far:

In the About section of the top nav menu, when you click on Working with the FEC, you're taken to the bottom of the page, but the menu on the left stays stuck at the top of the screen, so if you scroll up, it looks like this:

screen shot 2017-12-13 at 12 59 27 pm

In the Legal section of the top nav menu, clicking on Court Cases takes you to that page and the menu stays in place when you click and scroll around the page - is this expected?

screen shot 2017-12-13 at 1 08 03 pm

All that said, most of the code changes themselves are okay as far as I can tell now!

@xtine
Copy link
Contributor Author

xtine commented Dec 13, 2017

@ccostino: thanks for bringing up those bugs! so I realized the way I was "fixing" overscrolling caused weirdness on any sticky elements, including those side nav bars. So I found another solution that will still successfully hide glossary from overscrolling but doesn't touch the overflow settings of html/body to not mess around with the scroll event that was causing the problems before.

@ccostino
Copy link
Contributor

ccostino commented Dec 13, 2017

Thanks, @xtine! Things look okay to me now in poking around, but will do a final code review again too. :-)

@lbeaufort, please feel free to take a look and review too!

@jenniferthibault
Copy link
Contributor

Ah, @xtine I did find a thing that removing the sticky component broke: the left-hand-side jump menu, as seen on https://www.fec.gov/help-candidates-and-committees/filing-reports/

screen shot 2017-12-13 at 8 12 22 pm

@xtine
Copy link
Contributor Author

xtine commented Dec 14, 2017

The left sticky component shouldn’t be affected with my latest change.

Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

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

Thank very much, @xtine!

@ccostino ccostino merged commit 37093aa into develop Dec 15, 2017
@xtine xtine deleted the feature/minor-frontend-cleanup branch December 15, 2017 20:20
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.

4 participants