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

Fixes AO pagination #1383

Merged
merged 2 commits into from
Oct 25, 2017
Merged

Conversation

patphongs
Copy link
Member

Resolves https://github.com/18F/fec-cms/issues/1382

This sets the standard offset to 20 and accounts for boundaries in the from_hit value. This ensures no negative pages or pages that would go beyond the total result count.

@codecov-io
Copy link

codecov-io commented Oct 25, 2017

Codecov Report

Merging #1383 into release/public-20171011 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                    @@
##           release/public-20171011    #1383   +/-   ##
========================================================
  Coverage                    79.49%   79.49%           
========================================================
  Files                           46       46           
  Lines                         3297     3297           
  Branches                       497      497           
========================================================
  Hits                          2621     2621           
  Misses                         676      676

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 82dd27e...be55422. Read the comment docs.

function interceptHandleChange(offset) {

// from_hit is a zero-based index
var updatedFromHit = props.from_hit + offset;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is ES6, so I believe this needs to be let.

}

// create an event to update from_hit
const syntheticEvent = { target: { name: 'from_hit', value: updatedFromHit } };
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand the object in a nested fashion for readability?

var updatedFromHit = props.from_hit + offset;

// when from_hit is less than 0, reset to 0 (ensures no negative value)
if (updatedFromHit < 0){
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after ). I would recommend installing eslint or something similar.

Copy link
Contributor

@xtine xtine left a comment

Choose a reason for hiding this comment

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

This works, excellent work diving into the legal search code!

@xtine xtine merged commit 84c099a into release/public-20171011 Oct 25, 2017
@xtine xtine deleted the fix/1382-fix-ao-pagination branch October 25, 2017 18:04
@patphongs patphongs mentioned this pull request Oct 31, 2017
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.

3 participants