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

Update boolean search box and add tooltips for AO,MUR, Statutes #2574

Merged
merged 7 commits into from
Dec 17, 2018

Conversation

johnnyporkchops
Copy link
Contributor

@johnnyporkchops johnnyporkchops commented Dec 6, 2018

Summary

Make design/layout edits to keyword modal html and jinja templates
Add tooltip for keyword text-box field on AO, MUR, Statues

  • AO Search is React.js, so I made the tooltip in here, a reusable react component (Could now be
    used on any filter type inside AO search with custom tooltip message and positioning classes)

  • MUR and Statutes datatables are both rendered from legal.jinja, so the tooltip is hardcoded there

  • Resolves Update boolean search box and add tooltip #2283

Impacted areas of the application

AO Search react app
- modified: fec/static/js/legal/Filters.js
- modified: fec/static/js/legal/filters/TextFilter.js
- added: fec/static/js/legal/filters/TooltipHelp.js
MUR and Statutes datatables:
- modified: legal/templates/macros/legal.jinja
- modified: fec/static/scss/components/_tooltips.scss
Keyword Modal popup:
- modified: home/templates/partials/legal-keyword-modal.html
- modified: legal/templates/partials/legal-keyword-modal.jinja
- modified: fec/static/scss/components/_search-controls.scss
- modified: fec/static/scss/components/_modals.scss

Screenshots

Tested in Internet Explorer below, per checklist item 5 in issue:

gif2


How to test

Checkout this branch locally: feature/update-boolean-search-box
NPM run build
Runserver and test the tooltips on Keyword text filter on AO, MUR and Statutes searches
Test keyword modal box on the same pages as well as the legal landing page (this uses a different template)
If one were ambitious they could also test the reusable tooltip react component by adding it to DateFilter.js* by:

  • Copying the tooltip code from the TextFilter.js file:
    • Lline 3
    • Lines 21-27
    • Line 69 and line 79
      ...and pasting it into DateFilter.js.
  • Then enable it in Filters.js for that filter-type (DateFilter), like I did for the TextFilter (@line 56). Maybe add a different message.
  • Don't forget to run build, and see if it works! ( I tested it but would still like to see if someone else thinks it works well)

*For filters that depend on other filters, like CheckboxFiler, the process is different, but it can still be added to any filter in the panel.

@johnnyporkchops
Copy link
Contributor Author

johnnyporkchops commented Dec 6, 2018

@JonellaCulmer --There are still two minor design issues with this PR that I did not want to hold up the PR getting merged for testing.

  • Tooltips are positioned under the element. The classes we have for above positioning ( tooltp--above) are not working properly, and I need to take more time to understand how to fix this the right way.

  • In Internet Explorer, the "paper-stack" icon does not show adjacent to the keyword modal link for some reason.
    These can probably be fixed before you test, but just giving you a heads up.

@JonellaCulmer
Copy link
Contributor

JonellaCulmer commented Dec 7, 2018

Thanks for the heads up, @johnnyporkchops!

@JonellaCulmer
Copy link
Contributor

@johnnyporkchops I can review and approve this, or were you going to try to implement the other stuff before review/merge. Happy to do it either way. Up to you.

@codecov-io
Copy link

codecov-io commented Dec 12, 2018

Codecov Report

Merging #2574 into develop will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2574      +/-   ##
===========================================
- Coverage    74.98%   74.97%   -0.01%     
===========================================
  Files          115      115              
  Lines         6971     6970       -1     
  Branches       599      599              
===========================================
- Hits          5227     5226       -1     
  Misses        1744     1744
Impacted Files Coverage Δ
fec/fec/static/js/modules/tables.js 53.99% <0%> (-0.33%) ⬇️
fec/data/views.py 48.38% <0%> (+0.44%) ⬆️

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 027361b...d53bd31. Read the comment docs.

@johnnyporkchops
Copy link
Contributor Author

johnnyporkchops commented Dec 12, 2018

@JonellaCulmer. I pushed fix for outstanding checklist item 1 above. Here are screenshots of how the tooltips render now for this branch (not updated on feature space yet). The one on the Statutes and MUR pages needed to be shorter than the mockup to avoid overlapping with the header content. Is this visual inconsistency an issue from a UX POV? (The default width in the CSS for centered tooltips was narrower than you mockup, but we could probably make that a little wider)

AO Page:

screen shot 2018-12-11 at 9 10 06 pm


MUR Page overlap issue:

screen-shot-2018-12-11-at-9 20_overlap


MUR Page w/ overlap fixed:

screen shot 2018-12-11 at 9 11 08 pm

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.

Technical changes look good to me. @JonellaCulmer Once you complete your design review, please go ahead and merge

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 wonderful, @johnnyporkchops! Thanks for all your hard work on this!

@lbeaufort lbeaufort merged commit 1513942 into develop Dec 17, 2018
@lbeaufort lbeaufort deleted the feature/update-boolean-search-box branch June 21, 2019 20:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants