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.

Sign up for GitHub

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

Jump to bottom

Add additional fields to list view table #126

Merged
merged 6 commits into from
Oct 13, 2023
Merged

Add additional fields to list view table #126

merged 6 commits into from
Oct 13, 2023

Conversation

brianlove
Copy link
Contributor

@brianlove brianlove commented Oct 2, 2023 •
edited
Loading

Add all currently-available data fields in the articles, patents, and other_metrics keys to the List View table.

Closes #121, closes #131

@brianlove brianlove requested review from jmelot and za158 October 2, 2023 16:42
@brianlove brianlove self-assigned this Oct 2, 2023
@github-actions
Copy link

github-actions bot commented Oct 2, 2023 •
edited
Loading

No need for rebasing 👍
behind_count is 0
ahead_count is 6

@github-actions
Copy link

github-actions bot commented Oct 2, 2023 •
edited
Loading

JavaScript Coverage

Summary

Lines Statements Branches Functions
Coverage: 71%
 71.31% (276/387) 59.03% (98/166) 72.93% (97/133)
Modified Files • (71%)
File% Stmts% Branch% Funcs% LinesUncovered Line #s
All files71.3159.0372.9371.46 
components72.2561.5372.8972.29 
   AddRemoveColumnDialog.jsx91.665083.3391.377, 97
   CellStat.jsx10066.6610010032–35
   HeaderSlider.jsx86.661007585.7157–58
   ListViewTable.jsx91.017793.7592.21159, 190–191, 196–199, 250, 283–284, 318, 333, 416, 447, 482
static_data88.8810010088.88 
   table_columns.js100100100100 

jmelot
jmelot reviewed Oct 4, 2023
View reviewed changes
Copy link
Member

@jmelot jmelot left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just some comments about boilerplate (it's less maintainable and readable, I think we should wrap this up in iterators unless there's a compelling reason not to I'm not thinking of), I don't see any more substantive issues here

web/gui-v2/src/components/ListViewTable.jsx Outdated Show resolved Hide resolved
web/gui-v2/src/components/ListViewTable.jsx Outdated Show resolved Hide resolved
web/gui-v2/src/static_data/table_columns.js Show resolved Hide resolved
@jmelot
Copy link
Member

jmelot commented Oct 4, 2023 •
edited
Loading

When I tried this out in the UI I noticed two issues:

1.) AI pubs column is super wide for some reason

Screenshot 2023-10-04 at 3 34 12 PM

2.) When I add all cols, scroll all the way to the right, and sort by AI jobs, things get weird (see last two columns)

Screenshot 2023-10-04 at 3 35 07 PM

@brianlove brianlove mentioned this pull request Oct 5, 2023
Closed
@brianlove
Copy link
Contributor Author

RE the AI/TT1 jobs column weirdness: The main part of the weirdness looks to have been resolved by changing how null values are treated and displayed (previously they were sorted above all other values, now they're sorted at the other end with zero). The remaining issues can be resolved by adjusting the minimum column width.

@brianlove brianlove requested a review from jmelot October 5, 2023 20:27
jmelot
jmelot reviewed Oct 11, 2023
View reviewed changes
Copy link
Member

@jmelot jmelot left a comment

Choose a reason for hiding this comment

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

Looking much nicer! As discussed, one follow-on in #134 . I do have some styling notes which I'll follow up with in the comments

@jmelot
Copy link
Member

jmelot commented Oct 11, 2023 •
edited
Loading

Feel free to address in follow-on issues but:

Could use more padding in the last column

Screenshot 2023-10-11 at 10 05 29 AM

Dropdowns too narrow here (this is with all columns selected because I'm a pathological "user")

Screenshot 2023-10-11 at 10 06 47 AM

@brianlove
Add additional fields to list view table
b5b831f 
Add all currently-available data fields in the `articles`, `patents`,
and `other_metrics` keys to the List View table.

Closes #121
@brianlove
Handle null values and reduce boilerplate code
f34ad1d 
Treat null values as zero for sorting purposes; display as "n/a".
Reduce boilerplate code for default values.
@brianlove
Reduce more boilerplate
ff83781 
There's an issue with filter states "bouncing" - tracked in #131
@brianlove
Fix 'bouncing' states
11a906a 
Fix the "party parrot filters" issue with filters on page refresh

Closes #131
@brianlove
Simpify column definitions and increase minimum column width
c94e929 
Simplify the column definitions for slider columns to reduce necessary
boilerplate.  Set a minimum width for slider columns (and ensured that
it is actually applied) to reduce crowding in some of the smaller
columns (like AI/TT1 jobs).
@brianlove
Adjusted cell stat padding to accomodate 4-digit ranks
135a197 
Adjusted the layout of `<CellStat>` to ensure that there is always
enough space for a 4-digit ranking (plus "#").
@brianlove brianlove force-pushed the 121-integrate-list-view-data branch from 365dec3 to 135a197 Compare October 13, 2023 02:14
@brianlove
Copy link
Contributor Author

Addressed cell ranking spacing in this PR and tracking column dropdown width in #143.

@brianlove brianlove merged commit 2558281 into version2 Oct 13, 2023
3 checks passed
@brianlove brianlove deleted the 121-integrate-list-view-data branch October 13, 2023 02:25
@brianlove brianlove mentioned this pull request Oct 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmelot jmelot jmelot left review comments

@za158 za158 Awaiting requested review from za158

Assignees

@brianlove brianlove

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brianlove @jmelot