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.

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

Add rows to list view for selected groups #183

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

brianlove
Copy link
Contributor

Add rows to the list view displaying the average data for each selected group (S&P 500, Fortune 500, etc). Groups currently have placeholder data, but this will be resolved in #123.

Styling to distinguish group rows from company rows will require changes to the UI Components <Table>, which is tracked in https://github.com/georgetown-cset/eto-ui-components/issues/341

Closes #101

@brianlove brianlove requested a review from jmelot December 1, 2023 23:22
@brianlove brianlove self-assigned this Dec 1, 2023
Copy link

github-actions bot commented Dec 1, 2023

No need for rebasing 👍
behind_count is 0
ahead_count is 2

Copy link

github-actions bot commented Dec 1, 2023

JavaScript Coverage

Summary

Lines Statements Branches Functions
Coverage: 67%
68.18% (375/550) 60.34% (175/290) 67.91% (127/187)
Modified Files • (67%)
File% Stmts% Branch% Funcs% LinesUncovered Line #s
All files68.1860.3467.9167.83 
components64.5662.6662.564.01 
   ListViewTable.jsx94.0181.6398.3394.06324–325, 330–333, 425, 470, 625, 679–680, 683, 702, 722–727
static_data72.584876.6669.64 
   table_columns.js71.664876.6668.51190–217, 232–234, 257, 293–315, 330–332

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.

👍 - one small thing, not sure this was introduced here but I noticed if I selected a company group and a company, the total displayed below the table by the pagination control is 1 larger than it should be

Screenshot 2023-12-04 at 11 32 15 AM

@brianlove
Copy link
Contributor Author

For the "Viewing X of Y companies" at the top left, I'm displaying only the number of companies, not groups. The number in the pagination controls in the lower right is the total number of rows (companies + groups).

@jmelot
Copy link
Member

jmelot commented Jan 23, 2024

@za158 do you have an opinion here? The only thing that occurred to me was changing the word "companies" to "results" in "viewing 502 of 1670 companies", and then including the sum of the number of companies and company groups in that text

@za158
Copy link
Member

za158 commented Jan 23, 2024

If feasible, I would prefer to put the word "rows" after 503 in that example image.

We will need to change the heading of the first column to something other than "Company" if groups are selected there, but we can figure that out later.

@brianlove
Copy link
Contributor Author

Adding "rows" after "503" will require adding support for TablePagination's labelDisplayedRows prop to ETO <Table> (tracked in https://github.com/georgetown-cset/eto-ui-components/issues/362)

@brianlove
Copy link
Contributor Author

@za158 Adding the pagination controls does add a mention of rows to that part of the table, but in a slightly different spot. Do you think this screenshot is sufficient, or do you still want it to say "1-10 of 503 rows"?

image

@za158
Copy link
Member

za158 commented Feb 1, 2024

This seems fine

@brianlove brianlove force-pushed the 101-add-group-average-row-in-list-view branch from 67d2ba8 to bb3cf7d Compare February 1, 2024 17:58
@jmelot
Copy link
Member

jmelot commented Feb 9, 2024

Two more small things:

1.) If you click on the name of the average row, you are directed to an empty company page - we should just not link to anything
2.) Can you add " (average)" after the name of the company group? It's not obvious that the figures in the row are averages, otherwise

Add rows to the list view displaying the average data for each selected
group (S&P 500, Fortune 500, etc).  Groups currently have placeholder
data, but this will be resolved in #123.

Styling to distinguish group rows from company rows will require
changes to the UI Components `<Table>`, which is tracked in
georgetown-cset/eto-ui-components#341

Closes #101
@brianlove brianlove force-pushed the 101-add-group-average-row-in-list-view branch from bb3cf7d to 452d610 Compare February 9, 2024 19:44
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.

👍

@jmelot jmelot merged commit 65dea2e into version2 Feb 9, 2024
5 checks passed
@jmelot jmelot deleted the 101-add-group-average-row-in-list-view branch February 9, 2024 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants