-
Notifications
You must be signed in to change notification settings - Fork 0
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 table columns #154
Update table columns #154
Conversation
No need for rebasing 👍 |
JavaScript CoverageSummary
Modified Files • (61%)
|
f5aa4e6
to
a6d8b4a
Compare
Update table column definitions to match the latest data Closes #92
Add column definitions for remaining requested columns, adding notes about what is still needed (where relevant)
Updated tests to reflect the new selection of initial columns. Commented out the group selector test since that interface will be changing substantially in #94 and was impacted by the UI Components update.
Update row filtering to account for growth columns, which can have negative values. Updated column generation to prevent columns with similar keys from interfering with each other.
a6d8b4a
to
2b9372f
Compare
0439dc0
to
993c151
Compare
993c151
to
5a863d5
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.
Looks good to me, found one small bug
@@ -64,7 +64,7 @@ describe("ListView", () => { | |||
}, 90000); | |||
}); | |||
|
|||
describe('groups', () => { | |||
describe.skip('groups', () => { |
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.
Why skip?
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 the data update would have necessitated an update to the group selection test here, but that group selection interface will be changing entirely in #156, I opted to just disable the old test.
((_val, row) => { | ||
const data = row.articles.all_publications; | ||
const startVal = data.counts[startArticleIx]; | ||
return Math.round((data.counts[endArticleIx] - startVal) / startVal * 1000) / 10; |
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.
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.
That's one of the questions from #92 (comment):
- 5-year growth in papers
I can calculate the growth rate, but then we wouldn't have rankings. If we want rankings in this column (and the similar patents column) then this stat should be pre-calculated and included in the data
Related question: how should infinite growth be handled (i.e. 0 papers in the start year, and some number >0 in the end year)? Right now these rows say "Infinity%".
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.
Ugh sorry I missed that. Ok, let's just merge this and I'll come back to it
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'll pre-calculate this for you and include the rank
Update table column definitions to match the latest data, including notes about needed information/data where applicable.
Closes #92