-
Notifications
You must be signed in to change notification settings - Fork 204
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
Support filtering annotations by page number or range #5997
Conversation
fieldValues: ann => [pageLabel(ann)?.trim() ?? ''], | ||
matches: (pageLabel: string, pageTerm: string) => | ||
pageLabelInRange(pageLabel, pageTerm), | ||
normalize: (val: string) => val.trim(), |
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.
This code currently reparses the page numbers for every annotation that is matched. That's OK since the number of annotations is expected to be small in the grand scheme of things (hundreds at most). If this needs to be optimized then we can convert annotation page ranges to some pre-parsed representation.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## display-page-numbers-on-cards #5997 +/- ##
==============================================================
Coverage 99.44% 99.44%
==============================================================
Files 259 260 +1
Lines 9918 9941 +23
Branches 2375 2386 +11
==============================================================
+ Hits 9863 9886 +23
Misses 55 55 ☔ View full report in Codecov by Sentry. |
Support `page:{number}` or `page:{start}-{end}` filters in the sidebar. When the query is a number, it must match the page label exactly. When it is a range, it matches the page number if: - The start and end of the range, and page number, are all numeric - The page number is between the parsed `start` and `end` points, inclusive Part of #5937.
6658153
to
03aed2d
Compare
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.
LGTM. I just added a suggestion to reduce conditional nesting in one function.
src/sidebar/util/page-range.ts
Outdated
if (range.includes('-')) { | ||
let [start, end] = range.split('-'); | ||
if (!start) { | ||
start = label; | ||
} | ||
if (!end) { | ||
end = label; | ||
} | ||
const [startInt, endInt, labelInt] = [ | ||
parseInt(start), | ||
parseInt(end), | ||
parseInt(label), | ||
]; | ||
if ( | ||
Number.isInteger(startInt) && | ||
Number.isInteger(endInt) && | ||
Number.isInteger(labelInt) | ||
) { | ||
return labelInt >= startInt && labelInt <= endInt; | ||
} else { | ||
return false; | ||
} | ||
} else { | ||
return label === range; | ||
} |
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.
Perhaps we can simplify the conditions that need to be mentally dragged by adding an early return for the simplest condition, and adding the rest afterwards:
if (range.includes('-')) { | |
let [start, end] = range.split('-'); | |
if (!start) { | |
start = label; | |
} | |
if (!end) { | |
end = label; | |
} | |
const [startInt, endInt, labelInt] = [ | |
parseInt(start), | |
parseInt(end), | |
parseInt(label), | |
]; | |
if ( | |
Number.isInteger(startInt) && | |
Number.isInteger(endInt) && | |
Number.isInteger(labelInt) | |
) { | |
return labelInt >= startInt && labelInt <= endInt; | |
} else { | |
return false; | |
} | |
} else { | |
return label === range; | |
} | |
if (!range.includes('-')) { | |
return label === range; | |
} | |
let [start, end] = range.split('-'); | |
if (!start) { | |
start = label; | |
} | |
if (!end) { | |
end = label; | |
} | |
const [startInt, endInt, labelInt] = [ | |
parseInt(start), | |
parseInt(end), | |
parseInt(label), | |
]; | |
if ( | |
Number.isInteger(startInt) && | |
Number.isInteger(endInt) && | |
Number.isInteger(labelInt) | |
) { | |
return labelInt >= startInt && labelInt <= endInt; | |
} | |
return false; |
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.
I agree. I prefer the early-return version as long as that code path is very simple.
Support
page:{number}
orpage:{start}-{end}
filters in the sidebar. When the query is a number, it must match the page label exactly. When it is a range, it matches the page number if:start
andend
points, inclusiveIn the context of VitalSource assignments with page numbers, the plan is that this filtering will be used as part of a "focus on pages in range X-Y", which will be implemented internally similar to the way that "focus on user X" works when grading.
Part of #5937.