-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
feat: use rison for list view filters stateful urls #11675
Conversation
I'm confused, what in this change is actually making these filters "sticky"? |
Codecov Report
@@ Coverage Diff @@
## master #11675 +/- ##
==========================================
+ Coverage 62.86% 63.36% +0.50%
==========================================
Files 889 905 +16
Lines 43055 43891 +836
Branches 4015 4216 +201
==========================================
+ Hits 27066 27811 +745
- Misses 15811 15901 +90
- Partials 178 179 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@nytai might need to use a different name, I think the original understanding of this feature is different from the actual implementation! I'm going based on the associated issue title. It's more a re-working of the formatting and fixing some UI bugs. |
@riahk I figured as much. Do you think we could rename that ticket, or at the very least this PR, to something like |
@@ -78,6 +82,23 @@ export function convertFilters(fts: InternalFilter[]): FilterValue[] { | |||
.map(({ value, operator, id }) => ({ value, operator, id })); | |||
} | |||
|
|||
// convertFilters but to handle new decoded rison format | |||
export function convertFiltersRison(filterObj: any): FilterValue[] { |
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 noticed before that all unselected filters muddy the urls, can we filter out where value === undefined
?
Also did we consider going with a better/simpler data structure? maybe
{
user: ['==', 'mistercrunch'],
dataset: ['==', 'some_dataset'],
}
(dataset:!(==,some_dataset),user:!(==,mistercrunch))
OR
[
["user", "==", "mistercrunch"],
["dataset", "!=", "some_dataset"]
]
!(!(user,==,mistercrunch),!(dataset,'!!=',some_dataset))
You can play with structures and see resulting URLs here https://rison.io/
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 think the operators names were long too, and using things like ==
!=
IN
would make for nicer/shorter URLs
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.
yes, this update should only put non-default filters in the URL for a much cleaner experience. I'll add some screenshots that show the URL params next to the selected filters. Also want to remove the operators but I'm working through a weird bug around that atm.
faba359
to
3e38ca0
Compare
3423ba1
to
fe8c98a
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.
Looking good, just a couple suggestions
@@ -63,11 +68,11 @@ function updateInList(list: any[], index: number, update: any): any[] { | |||
]; | |||
} | |||
|
|||
function mergeCreateFilterValues(list: Filter[], updateList: FilterValue[]) { | |||
function mergeCreateFilterValues(list: Filter[], updateObj: any) { |
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.
can we type the query filters object? something like
type QueryFilterState = {
[id: string]: [value: FilterValue["value"], operator: FilterValue["value"]]
}
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'm also going to be modifying this file in #11702 so I'll make this change when I rebase after this is merged.
import { BrowserRouter as Router, Route } from 'react-router-dom'; | ||
import { QueryParamProvider } from 'use-query-params'; | ||
|
||
// Wrapper component for mounting TODO: move to util file |
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.
Would it be possible to address this TODO before merge?
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.
This appears to be a util file, I think this comment just needs to be removed
SUMMARY
ListView
logic filter logic, update URLfilters
param to use a rison formatfilters
to only include filters set to non-default values and rm irrelevant dataFilters
UI component to properly populate selected dropdown filter (for non-async filter options)viewMode
URL paramBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
use-query-params
libstyledMount
/styledShallow
helper functions to includeRouter
andQueryParamProvider
ADDITIONAL INFORMATION