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

Narrow filter options to only those still applicable given the applied filters #48

Merged
merged 4 commits into from
Aug 24, 2023
Merged

Narrow filter options to only those still applicable given the applied filters #48

merged 4 commits into from
Aug 24, 2023

Conversation

brianlove
Copy link
Contributor

Limit the filter options present in dropdowns to only those that are still possible given the currently-applied filters from other columns.

Closes #37

@brianlove brianlove requested a review from jmelot August 16, 2023 22:05
@brianlove brianlove self-assigned this Aug 16, 2023
@github-actions
Copy link

github-actions bot commented Aug 16, 2023 •
edited
Loading

No need for rebasing 👍
behind_count is 0
ahead_count is 4

@github-actions
Copy link

github-actions bot commented Aug 16, 2023 •
edited
Loading

JavaScript Coverage

Summary

Lines Statements Branches Functions
Coverage: 65%
 64.33% (193/300) 48.64% (54/111) 71.42% (75/105)
Modified Files • (65%)
File% Stmts% Branch% Funcs% LinesUncovered Line #s
All files64.3348.6471.4265.48 
components72.0155.2976.7172.59 
   ListView.jsx100100100100 
   ListViewTable.jsx88.0966.1592.389.83141–142, 163–164, 169–176, 224, 247, 327–332

@brianlove brianlove linked an issue Aug 21, 2023 that may be closed by this pull request
Narrow filter options based on other filters #37
Closed
@brianlove brianlove force-pushed the 37-narrow-filters-based-on-applied-filters branch from f4d40c6 to 4b290c6 Compare August 21, 2023 16:49
@jmelot jmelot force-pushed the 37-narrow-filters-based-on-applied-filters branch 3 times, most recently from 5e1f575 to 2b30b57 Compare August 22, 2023 14:36
jmelot
jmelot reviewed Aug 22, 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.

The code looks ok to me, but there's a bug somewhere. If I select filters like so:

Screenshot 2023-08-22 at 10 38 16 AM

and then select the company name, things break:

Screenshot 2023-08-22 at 10 38 21 AM

Mind you, this is a bit of pathological user behavior, I doubt we'd see this often/ever in the wild. But we should still fix it!

web/gui-v2/src/components/ListViewTable.jsx
return false;
}
} else if ( colDef.type === "stock" ) {
// TODO: Figure out how we're filtering the `market_list` column
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we have market_list and full_market_links and market_filt. I think you already found this but this is the set of exchanges we filter to: https://github.com/georgetown-cset/parat/blob/version2/web/scripts/retrieve_data.py#L63C1-L63C15

@brianlove brianlove marked this pull request as draft August 22, 2023 15:00
@brianlove
Narrow filter options to only those still applicable
2f12168 
Limit the filter options present in dropdowns to only those that are
still possible given the currently-applied filters from other columns.

Closes #37
@brianlove brianlove force-pushed the 37-narrow-filters-based-on-applied-filters branch from 2b30b57 to 2f12168 Compare August 22, 2023 15:03
@brianlove brianlove marked this pull request as ready for review August 22, 2023 15:04
@brianlove
Encode commas so that names with commas aren't split apart
228401a 
Convert commas into HTML entities before storing them in the URL query
params so that when the filters are split (on commas), the company name
stays intact.
@brianlove brianlove requested a review from jmelot August 22, 2023 16:14
jmelot
jmelot approved these changes Aug 22, 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.

👍

@jmelot
Copy link
Member

jmelot commented Aug 22, 2023

Some weirdness with the dropdown, I didn't check if related
Screenshot 2023-08-22 at 5 29 34 PM
Screenshot 2023-08-22 at 5 29 50 PM

@jmelot
Copy link
Member

jmelot commented Aug 22, 2023

Also, if I specify a stage and a country, then select s&p, this happens

Screenshot 2023-08-22 at 5 32 47 PM

@brianlove
Give all filter options when filtered result is empty
983a353 
When the currently-applied filters give no results, leave all possible
filter options available in the dropdowns in order to give the user
the most flexibility in adjusting filters to get results.
@brianlove brianlove marked this pull request as draft August 23, 2023 21:07
@brianlove
Add fallback text when no rows match filters
60353c2 
Update UI Components to be able to provide `fallbackContent` when the
list view table has no rows to display for the selected filters.
@brianlove brianlove force-pushed the 37-narrow-filters-based-on-applied-filters branch from 1cd44a6 to 60353c2 Compare August 24, 2023 17:25
@brianlove brianlove marked this pull request as ready for review August 24, 2023 17:34
@brianlove brianlove merged commit c174b7a into version2 Aug 24, 2023
3 checks passed
@brianlove brianlove deleted the 37-narrow-filters-based-on-applied-filters branch August 24, 2023 17:37
@brianlove brianlove mentioned this pull request Aug 24, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmelot jmelot jmelot approved these changes

Assignees

@brianlove brianlove

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Narrow filter options based on other filters
2 participants
@brianlove @jmelot