-
Notifications
You must be signed in to change notification settings - Fork 2
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
move dataset_split and task filter to system table header #528
Conversation
filters: Record<string, FilterValue | null>, | ||
_sorter: SorterResult<SystemModel> | SorterResult<SystemModel>[] | ||
) => { | ||
console.log(filters); |
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.
Could you please remove the console log?
_sorter: SorterResult<SystemModel> | SorterResult<SystemModel>[] | ||
) => { | ||
console.log(filters); | ||
for (const k in filters) { |
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 for loop has a couple of minor bugs/typos. I think we can fix them and simplify the code to something like this
const filterUpdate: Partial<SystemFilter> = {};
const dataset_split = filters["dataset.split"]
? (filters["dataset.split"][0] as string)
: undefined;
if (dataset_split !== filterValue.split) {
filterUpdate.split = dataset_split;
}
const task = filters["task"] ? (filters["task"][0] as string) : undefined;
if (task !== filterValue.task) {
filterUpdate.task = task;
}
onFilterChange(filterUpdate);
Note that I changed the following things:
- I don't think we need a for loop here. Loops are for repetitive things but we want to handle each key differently here.
filters["task"]
andfilters["dataset.split"]
are arrays if the filters are active. I don't think we can typecast them to string.- To check whether the new filter value is different from the current one, I converted filters[key] to the value we want with an assignment and then this check is just an equality check.
onFilterChange()
is only called once at the end. This makes it easier to track the states becausefilterValue
is only updated once.
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.
Thank you very much! This looks much clearer. I add another boolean flag before onFilterChange
because onFilterChange
will set the pagination to page one even when filterUpdate
is empty.
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.
Looks great! Thanks!
This is a sub-component of PR #518
Move dataset split and task filter to the system table header