feat: IESG table column 'Pre pubreq'#7462
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## feat/ad-dash #7462 +/- ##
===============================================
Coverage ? 88.79%
===============================================
Files ? 303
Lines ? 41412
Branches ? 0
===============================================
Hits ? 36771
Misses ? 4641
Partials ? 0 ☔ View full report in Codecov by Sentry. |
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
One inline comment. Also, as a general matter, it'd be nice to keep chipping away at moving javascript out of inline <script> tags and into their own js files. It's not exactly on-topic for this PR, though.
| cell | ||
| ) => { | ||
| const key = cell.dataset.sum | ||
| const value = parseFloat(cell.textContent) |
There was a problem hiding this comment.
May want to check more aggressively that cell.textContent is a float given parseFloat("1NaN") = 1.0. It's a little paranoid since these cells are under control, but it might help someone who unwittingly breaks the structure in a future change. (Could perhaps be a test rather than a run-time check if this gets covered by tests.)
There was a problem hiding this comment.
So something like this?
function safeParseFloat(text) {
const trimNumber = text.trim()
if(!trimNumber.match(/^[0-9.]+$/)) {
console.warn(`Unable to parse "${trimNumber}" as a number.`)
return Number.NaN
}
return parseFloat(text)
}I think having tests for this is going to be hard with inline scripts but I guess I could put it into Playwright
There was a problem hiding this comment.
I'll try refactoring this JS into a separate script in the next PR.
There was a problem hiding this comment.
That would do it, though we could also do it on the Django side just looking at the rendered template. That might be easier to turn into a test failure without having to add Playwright coverage or resort to console.warn.
| const viewports = require('../../helpers/viewports') | ||
|
|
||
| // ==================================================================== | ||
| // NOMCOM - EXPERTISE |
5 participants
Fixes #4577