Add type based filtering, improve filtering UX #1341

pavish · 2022-05-02T21:46:05Z

This PR:

Updates filtering to use latest request structure
Adds type specific filtering options
Adds type based filter inputs
Optimizes Cell performance by calculating abstract type info and input info at a higher layer
Improves filtering UX and error states

Screen.Recording.2022-05-03.at.3.03.43.AM.mov

Note: This PR does not use the filtering endpoint as mentioned in the ticket. This is because the filtering API lacks information the frontend requires, which we decided was not a good use of time to work on before the alpha. The response is now stubbed in the frontend, similar to Mathesar type configurations. We'll create a separate issue for it to work on after alpha.

I've not added tests for filtering yet. I intend creating new issues and working on them during the cool down phase.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the master branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no
visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

…filtering_1072

… cells to a higher layer. Improve typing.

…format

…rganize record action components

…filtering_1072

codecov-commenter · 2022-05-02T22:27:49Z

Codecov Report

Merging #1341 (a06197a) into master (73f785b) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1341   +/-   ##
=======================================
  Coverage   93.71%   93.71%           
=======================================
  Files         114      114           
  Lines        4439     4439           
=======================================
  Hits         4160     4160           
  Misses        279      279

Flag	Coverage Δ
pytest-backend	`93.71% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73f785b...a06197a. Read the comment docs.

…filtering_1072

seancolsen

This is awesome! The UX improvements are great, and it's really exciting to see type-specific options for the filter entries.

I'm merging, but I've also noted some lower-priority code questions and feedback that we can address later.

seancolsen · 2022-05-03T15:43:49Z

mathesar_ui/src/stores/table-data/filtering.ts


 export class Filtering {
-  readonly combination: FilterCombination;
+  combination: FilterCombination;


I notice you've removed readonly in a number of places. Why? It's not a big deal -- I'm just curious. My intent when building these low-level data structures was to make them immutable. I'm aware that TypeScript's readonly marker isn't perfect, but I like how it communicates intent (and goes some of the way towards insuring immutability).

Ahh. I forgot to revert this. At one point I was trying to make the entire Filter component reactive towards meta.filtering when the Dropdown is open, and I needed to allow manually setting these values without triggering a full rerender of the component.

We can safely revert this. I'll get to it in a future PR.

seancolsen · 2022-05-03T16:01:00Z

mathesar_ui/src/components/cell/utils.ts

@@ -34,18 +34,19 @@ function getCellConfiguration(
  };
 }

-export function getCellComponentWithProps(
+export function getCellCap(


Why the shortened name here? Is "CAP" Supposed to be an acronym for "Component And Props"? I would find this naming confusing, especially because "cap" is also a word with its own meaning. I much prefer the more verbose name.

Is "CAP" Supposed to be an acronym for "Component And Props"

Yes, it is. Keeping it verbose made it long, and I thought we could establish the term CAP in our code base as an acronym for Component And Props, since we don't use "cap" anywhere else. But I'm fine with keeping it verbose as well.

seancolsen · 2022-05-03T16:04:54Z

mathesar_ui/src/components/sheet/types.ts

+export interface SheetColumn {
+  column: Column;
+  abstractTypeOfColumn: AbstractType;
+  cellCap: ReturnType<typeof getCellCap>; // CAP refers to ComponentAndProps


As mentioned in my other comment, I don't like this abbreviation. I'd rather write ComponentAndProps for better readability.

If there's an important reason for keeping the "CAP" naming convention though, the code comment would be better as a JSDoc comment above the cellCap property so that it will be semantically associated and appear in editor tooltips elsewhere.

seancolsen · 2022-05-03T16:13:34Z

mathesar_ui/src/sections/table-view/actions-pane/record-operations/FilterEntry.svelte

+<style lang="scss">
+  .filter-entry {
+    display: flex;
+    min-width: 560px;


It seems a lot of this CSS is new. Is there a reason you've used px instead of em or rem, per our standard?

I'm treating most of this css as temporary since we probably might have to re-work some/most of the filtering, sorting and grouping when working on views. I used px to finish this quick. I'm waiting for the UI milestone to work purely on the styles in the entire product.

seancolsen · 2022-05-03T16:20:15Z

mathesar_ui/src/sections/table-view/actions-pane/record-operations/FilterEntry.svelte

+  export let columnId: FilterEntry['columnId'] | undefined;
+  export let conditionId: FilterEntry['conditionId'] | undefined;
+  export let value: FilterEntry['value'] | undefined;
+  export let noOfFilters: number;


I prefer longer, more readable names without abbreviations like numberOfFilterEntries or filterEntryQuantity or filterEntryCount.

seancolsen · 2022-05-03T16:25:47Z

mathesar_ui/src/sections/table-view/actions-pane/record-operations/FilterEntries.svelte

+  }>();
+
+  export let entries: FilterEntry[];
+  export let processedTableColumnsMap: ProcessedTableColumnMap;


For code readability and reduced mental friction, I'd ideally like to see consistent pluralization of "columns" across ProcessedTableColumnMap and processedTableColumnsMap

I agree. Seems like I didn't pay much attention while naming the interface.

pavish added 13 commits April 29, 2022 04:37

Specify filtering configuration on the frontend

b45e18f

Merge branch 'master' of https://github.com/centerofci/mathesar into …

3d42cf7

…filtering_1072

Merge branch 'master' of https://github.com/centerofci/mathesar into …

f9704de

…filtering_1072

Improve performance by moving calculation of components and props for…

0ae040b

… cells to a higher layer. Improve typing.

Update record store and filtering meta class based on latest request …

2b15257

…format

Update row & action item components to use pre-processed columns, reo…

1d14a96

…rganize record action components

Merge branch 'master' of https://github.com/centerofci/mathesar into …

4ca919c

…filtering_1072

Retain class in SimplifiedInputProps

91c37ad

Improve filter ux and styling

4772c6b

Merge branch 'master' of https://github.com/centerofci/mathesar into …

dcfc7a8

…filtering_1072

Show error indications

bdd8d9c

Retain previous styles for Sorting

e6dfd5b

Fix but where filter combination was not synced

3e3a456

pavish requested review from a team and seancolsen and removed request for a team May 2, 2022 21:46

pavish added the pr-status: review A PR awaiting review label May 2, 2022

pavish assigned seancolsen May 2, 2022

pavish requested review from a team and dmos62 and removed request for a team May 2, 2022 22:05

Fix failing e2e test

748697c

pavish force-pushed the filtering_1072 branch from 0ca3a10 to 748697c Compare May 2, 2022 22:07

Merge branch 'master' of https://github.com/centerofci/mathesar into …

a06197a

…filtering_1072

seancolsen approved these changes May 3, 2022

View reviewed changes

seancolsen merged commit 275240a into master May 3, 2022

seancolsen deleted the filtering_1072 branch May 3, 2022 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add type based filtering, improve filtering UX #1341

Add type based filtering, improve filtering UX #1341

pavish commented May 2, 2022 •

edited

Loading

codecov-commenter commented May 2, 2022 •

edited

Loading

seancolsen left a comment

seancolsen May 3, 2022

pavish May 3, 2022

seancolsen May 3, 2022

pavish May 3, 2022

seancolsen May 3, 2022

seancolsen May 3, 2022

pavish May 3, 2022

seancolsen May 3, 2022

seancolsen May 3, 2022

pavish May 3, 2022

Add type based filtering, improve filtering UX #1341

Add type based filtering, improve filtering UX #1341

Conversation

pavish commented May 2, 2022 • edited Loading

This PR:

Checklist

Developer Certificate of Origin

codecov-commenter commented May 2, 2022 • edited Loading

Codecov Report

seancolsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavish commented May 2, 2022 •

edited

Loading

codecov-commenter commented May 2, 2022 •

edited

Loading