-
Notifications
You must be signed in to change notification settings - Fork 21
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
Refactor results page graph tabs data formatting strategies and add analytics call #2196
Conversation
fdbc8ab
to
356f808
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.
Please check my comments next to the citation/reads
src/js/wraps/graph_tabs.js
Outdated
return; | ||
const counts = apiResponse.get('facets.citation_count.buckets'); | ||
|
||
let finalData = []; |
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.
@thostetler I'm not sure about this part: facet.limit=2000 will limit results returned to 2000 rows; each of which may contain values >1
-- so at max we'll have 2000 datapoints for the graph, but having less than 2000 should be also OK; padding it to be 2K will make the graph somewhat normalized, but also will shrink it somewhat -- so I'm personally inclined to say no padding is needed.
The numRecords < MAX_RECORDS
is not what we should do; It's OK to display 2000 datapoints even though numRecords will grow higher -- unless I'm missing something?
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'm not sure which facet.limit=2000
I took that part from your original comment.
I agree with you, I thought it was unnecessary to reduce the data points, however that was how it was implemented before (
bumblebee/src/js/wraps/graph_tabs.js
Line 177 in edf3ec4
// a cut off of 2000 |
So I just did the same thing again using the new non-pivot incoming data.
I've added a commit that removes that loop and just maps the data directly from solr: 64ca10f
src/js/wraps/graph_tabs.js
Outdated
const record = { x: i, y: val }; | ||
|
||
// create duplicate entries to pad out the array | ||
finalData = finalData.concat( |
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.
the same as above, with an additional comment that we should do finalData.push....
to avoid creating the array over and over which would be inefficient
as weird as it sounds, the data coming from solr can be used without modification
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.
Thanks, I think now it's perfect -- only some failing unittests (if you could please look at those). AFAIK it's in good shape and can be merged. Thanks for the analytics events too.
356f808
to
64ca10f
Compare
64ca10f
to
a191c7d
Compare
Adds
graph-tab-active [Citations / Reads]
GA interaction event for when tab is selectedImplements strategy laid out here: #2194 (comment)