-
Notifications
You must be signed in to change notification settings - Fork 43
Rulemakings keyword search doc highlights #6932
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
base: develop
Are you sure you want to change the base?
Conversation
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.
@johnnyporkchops Thanks for your work on this. On my initial look, this is what I've found so far. I'll take a closer look next week.
fec/fec/static/js/modules/columns.js
Outdated
| } | ||
| } | ||
| } | ||
| return eligible_documents.map(doc => `<strong>${doc}</strong><br>Comment deadline: ${comment_deadline}<br><a class="button--cta" href="/legal/rulemakings/${row.rm_no}/add-comments/">Submit a comment</a>`).join(''); |
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.
Since there is the potential of multiple documents being open for comment at once, we should think about making each of these buttons unique somehow. Maybe using the document ID /legal/rulemakings/${row.rm_no}/[doc_id]/add-comments/". If we do this though, we'll probably need to tweak the logic of the rulemaking comment application as well. But first, let's confirm with stakeholders if this is a potential issue or if there will only ever be one document open for comment at once. If there will only be one, then we can scrap the idea of having multiple comment buttons.
| $('#search-input').on('change', function(e) { | ||
| handleKeywordSearchChange(e); | ||
| }); | ||
|
|
||
| let new_val; | ||
| const handleKeywordSearchChange = function(e) { | ||
| const newVal = e.target.value; | ||
| new_val = e.target.value; |
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.
When you enter a keyword search and then do a page refresh, you'll see that q=undefined in the URL. It appears that this new_val variable is declared but never initialized?
…ight() function to clean highlight, updated comment button URL to add doc id, added import of legal-search styles for highlights, made the extra empty <th> column conditional
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6932 +/- ##
========================================
Coverage 80.20% 80.20%
========================================
Files 253 253
Lines 6324 6324
========================================
Hits 5072 5072
Misses 1252 1252 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…simplified some logic and cleaned up spacing / console debug statements
Summary (required)
Resolves #issue_number
Keyword search document highlights
Proximity search document display
Required reviewers
(Include who is required to review prior to merge. For example: One designer and two front end developer reviews are required prior to merge)
Impacted areas of the application
General components of the application that this PR will affect:
Rulemakings datatable
Screenshots
(Include a screenshot of the new/updated features in context (“in the wild”). If it is an interface change, include both before and after screenshots)
Related PRs
Related PRs against other branches:
How to test
(Include any information that may be helpful to the reviewer(s). This might include links to sample pages to test or any local environmental setup that is unusual such as environment variable (never credentials), API version to point to, etc)
System architecture updates (if applicable)
(If this pull request changes our current system diagram, include a description of those changes here and create a new ticket to update the system diagram)