fix(column-configurator): convert to using the taxonomic filter #8726

pauldambra · 2022-02-23T09:29:57Z

Changes

Loading the column configurator shows 0 available columns despite there being properties available

I can't reproduce this locally but am assuming that property definitions isn't triggering change detection because we're updating the same array instance not copying it

This change copies the array

related to #8257

How did you test this code?

By pushing to CI :/

mariusandra

Pretty sure this won't solve things. We just don't have the properties loaded. The right answer is to make this async, without requiring a download of 100MB of JSON.

pauldambra · 2022-02-24T10:02:12Z

## on a wide screen

## on narrower screens

maintains behaviour for pop-ups in existing contexts but changes them to close on mouse leave and changes default position to left so that the selected columns in the configurator are more visible

…gurator opens

pauldambra · 2022-02-27T08:37:20Z

Looking at this funnel less than 50% of people who open the column customizer choose a new column and then click save

The hypothesis would be that this change would mean it was over 50%

So the tradeoff of losing global filtering over the columns is worthwhile

mariusandra

Please downscale this PR and leave just the essentials. My main feedback:

There's no need for the popover at all. You can't interact with it and it just looks odd. Plus having it makes the code more complicated than it needs to be (all that new prop passing indirection and useEffect confusion). Let's also try to avoid passing the style prop.
The height of the left list is less than the right one
The title "Selected columns" is also misleading.

Small PRs. MVPs. Keep things simple. Don't add things just in case. Etc. :)

clarkus · 2022-03-01T15:51:13Z

This is kind of old, but I put together a version of this a while ago that would scale to various viewport sizes. https://www.figma.com/file/9yWtngNb1AIuf6KmXaEPJA/App-doodles?node-id=1512%3A146391

… fill its parent container

mariusandra · 2022-03-03T08:45:52Z

Not sure if this was ready for a review (please always re-tag), but had a quick poke anyway. The reset button is acting funky:

pauldambra · 2022-03-03T10:06:47Z

Not sure if this was ready for a review (please always re-tag),

@mariusandra it wasn't, no... CSS fighting me :)

I'll check the reset button

pauldambra · 2022-03-04T11:35:52Z

The reset button still does not work. I also find it strange that it closes the modal directly, instead of resetting the state and letting me choose what columns I want to show.

Very much not the scope of this PR 🤣

But also a small change to make it leave the modal open. So 🤷

In your gif with the double time column. You have time as a selected column in the modal. and the table always adds time on the end of the selected columns. I'm not sure which of those is a bug... But that's the behaviour in master so it's not introduced in this PR... Since this started as a 6 character PR have logged this here #8861

…earch query when an item is selected

pauldambra · 2022-03-04T14:37:46Z

mariusandra

Still to do

mariusandra · 2022-03-04T15:02:36Z

frontend/src/lib/components/TaxonomicFilter/InfiniteList.tsx

@@ -268,6 +268,7 @@ export function InfiniteList(): JSX.Element {
        totalResultCount,
        totalListCount,
        expandedCount,
+        showPopper,


Perhaps popper -> popover ?

"Popper" is just the name of the library, but "popover" (or "popup"?) is what we want to show.

mariusandra · 2022-03-04T15:03:10Z

frontend/src/lib/components/TaxonomicFilter/InfiniteSelectResults.tsx

@@ -56,7 +58,7 @@ export function InfiniteSelectResults({
        return (
            <BindLogic
                logic={infiniteListLogic}
-                props={{ ...taxonomicFilterLogicProps, listGroupType: taxonomicGroupTypes[0] }}
+                props={{ ...taxonomicFilterLogicProps, listGroupType: taxonomicGroupTypes[0], popperEnabled }}


Why can't this popperEnabled just be inside taxonomicFilterLogicProps? I mean... it already is there...?

mariusandra · 2022-03-04T15:05:00Z

frontend/src/lib/components/TaxonomicFilter/TaxonomicFilter.tsx

+    const style = {}
+    if (height) {
+        style['height'] = `${height}px`
+    }
+    if (width) {
+        style['width'] = `${width}px`
+    }


If the unit is pixels, with width and height you can just pass a number, and px will be added automatically. Also using flexible destructuring could make it a bit easier to read... maybe:

const style = { ...(width ? { width } : {}), ...(height ? { height } : {}), }

mariusandra · 2022-03-04T15:06:32Z

frontend/src/lib/components/PropertyFilters/components/taxonomicPropertyFilterLogic.test.ts

@@ -14,6 +15,7 @@ describe('the taxonomic property filter', () => {

    beforeEach(() => {
        initKeaTests()
+        columnConfiguratorLogic({ selectedColumns: [] }).mount()


Hmm... Will this test fail without it? If so, it's coupled in a strange way. Something to improve?

it's too yucky, I'll remove

mariusandra · 2022-03-04T15:08:20Z

frontend/src/lib/components/TaxonomicFilter/infiniteListLogic.ts

        index: [
-            0 as number,
+            (props.popperEnabled === false ? -1 : 0) as number,


Actually... this doesn't make sense: "If you disable the popup we unselect the first item.". This is a side-effect of popperEnabled that's really not obvious. Would a separate prop like selectFirstItem = false make more sense?

mariusandra · 2022-03-04T15:09:07Z

frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.test.ts

+        it('clears the search query when the column configurator saves', async () => {
+            await expectLogic(logic, () => {
+                logic.actions.setSearchQuery('tomato')
+                columnConfiguratorLogic.actions.save()
+            }).toMatchValues({
+                searchQuery: '',
            })
+        })


Wouldn't this fit better into a column configurator test? This taxonomic filter should be totally oblivious to where it's being used from.

mariusandra · 2022-03-04T15:10:29Z

frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx

@@ -49,6 +50,7 @@ export const taxonomicFilterLogic = kea<taxonomicFilterLogicType>({
            groupPropertiesModel,
            ['allGroupProperties'],
        ],
+        actions: [columnConfiguratorLogic, ['save']],


Oohh... no, this won't do. It's like a textfield has a dependency on a settings page. It should be the other way around.

Yeah, I've been sat chewing this over. There are some frustrating consequences of taxonomic filter being inside the column configurator that I don't seem to be able to squash

mariusandra · 2022-03-04T15:11:49Z

frontend/src/lib/components/TaxonomicFilter/taxonomicFilterLogic.tsx

        searchQuery: [
            '',
            {
                setSearchQuery: (_, { searchQuery }) => searchQuery,
-                selectItem: () => '',
+                selectItem: (state) => (props.clearSearchOnSelection === false ? state : ''),


Reducers should technically be pure functions. That means the same state + payload combo should always return the same value. Here we don't know if props has changed in between. Won't block for it, just remarking on the code smell :).

mariusandra · 2022-03-04T15:12:28Z

frontend/src/lib/components/TaxonomicFilter/types.ts

 }

 export type TaxonomicFilterValue = string | number

 export interface TaxonomicFilterLogicProps extends TaxonomicFilterProps {
    taxonomicFilterLogicKey: string
+    clearSearchOnSelection?: boolean


The interface this extends already contains this.

This reverts commit ce40e3a.

This reverts commit e91c407.

…r is saved" This reverts commit 21500ad.

…n react

pauldambra · 2022-03-07T08:47:38Z

mariusandra

I think you've worked enough on this 😅

copy array in selector to trigger change detection

d121a0f

pauldambra requested a review from mariusandra February 23, 2022 09:31

alexkim205 approved these changes Feb 23, 2022

View reviewed changes

mariusandra requested changes Feb 23, 2022

View reviewed changes

use taxonomic filter for the column configurator

689b2e8

fix column configurator tests

10770fc

pauldambra changed the title ~~Copy array in selector to trigger change detection~~ Convert the column configurator to use taxonomic filter Feb 24, 2022

pauldambra added 4 commits February 25, 2022 08:13

fix weird test failure

de18065

ensure the taxonomic popup isn't showing row 0 when then column confi…

16bff88

…gurator opens

rename visible columns now there are no hidden columns

d135e1a

rename visible columns now there are no hidden columns

df30ad3

pauldambra requested a review from mariusandra February 25, 2022 08:35

pauldambra added 2 commits February 26, 2022 09:02

tidy up column configurator

ea8bb34

which allows removal of a use of the property definitions model

ebddc4a

pauldambra requested a review from clarkus February 27, 2022 08:38

clarkus approved these changes Feb 28, 2022

View reviewed changes

mariusandra requested changes Mar 1, 2022

View reviewed changes

pauldambra added 6 commits March 2, 2022 13:23

Merge branch 'master' into empty-columns

f674c28

more specific prop to disable popper on taxonomic filter

757e7d6

better column name

e389bee

don't allow duplicate column selections

918f9ab

Merge branch 'master' into empty-columns

7e730af

use autosizer height and width and flex display to have infinite list…

6ddc641

… fill its parent container

set text overflow for long rows in the infinite list

5587ae4

pauldambra changed the title ~~Convert the column configurator to use taxonomic filter~~ fix(column-configurator): convert to using the taxonomic filter Mar 3, 2022

pauldambra added 2 commits March 4, 2022 11:18

limit scope of change to propertykeyinfo width

0bae446

alter column configurator reset behaviour

32c17c0

pauldambra added 4 commits March 4, 2022 11:49

set popper enabled without breaking text search

ac1d347

don't need to pass popper enabled two different ways to infinite list

016f866

allow taxonomic filter value consumers to configure it not to clear s…

bf26aea

…earch query when an item is selected

clear the taxonomic filter search when the column configurator is saved

21500ad

pauldambra requested a review from mariusandra March 4, 2022 14:42

fix tests

e91c407

mariusandra requested changes Mar 4, 2022

View reviewed changes

pauldambra added 7 commits March 4, 2022 15:31

default column configurator to an empty array of columns

ce40e3a

Revert "default column configurator to an empty array of columns"

ceb69c8

This reverts commit ce40e3a.

Revert "fix tests"

2198563

This reverts commit e91c407.

Revert "clear the taxonomic filter search when the column configurato…

b040f04

…r is saved" This reverts commit 21500ad.

rename popper to popover

1755198

move taxonomic filter config onto its props

ce85c12

tidier declaration of style

b0ead05

clarkus approved these changes Mar 4, 2022

View reviewed changes

pauldambra added 2 commits March 4, 2022 18:02

make selecting the first item explicit

1ab117b

always clear taxonomic filter on select but do it so infinite list ca…

3504c00

…n react

pauldambra force-pushed the empty-columns branch from c338d0e to 3504c00 Compare March 4, 2022 19:08

pauldambra added 2 commits March 4, 2022 19:19

fix tests

a3232d1

use the constant for no item selected, since it is available

e0653e6

pauldambra requested a review from mariusandra March 7, 2022 08:47

mariusandra approved these changes Mar 7, 2022

View reviewed changes

mariusandra merged commit 717beeb into master Mar 7, 2022

mariusandra deleted the empty-columns branch March 7, 2022 09:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(column-configurator): convert to using the taxonomic filter #8726

fix(column-configurator): convert to using the taxonomic filter #8726

pauldambra commented Feb 23, 2022 •

edited

Loading

mariusandra left a comment •

edited

Loading

pauldambra commented Feb 24, 2022

pauldambra commented Feb 27, 2022 •

edited

Loading

mariusandra left a comment •

edited

Loading

clarkus commented Mar 1, 2022

mariusandra commented Mar 3, 2022

pauldambra commented Mar 3, 2022

pauldambra commented Mar 4, 2022 •

edited

Loading

pauldambra commented Mar 4, 2022

mariusandra left a comment

mariusandra Mar 4, 2022

mariusandra Mar 4, 2022 •

edited

Loading

mariusandra Mar 4, 2022

mariusandra Mar 4, 2022

pauldambra Mar 4, 2022

mariusandra Mar 4, 2022

mariusandra Mar 4, 2022

mariusandra Mar 4, 2022

pauldambra Mar 4, 2022

mariusandra Mar 4, 2022

mariusandra Mar 4, 2022

pauldambra commented Mar 7, 2022

mariusandra left a comment

fix(column-configurator): convert to using the taxonomic filter #8726

fix(column-configurator): convert to using the taxonomic filter #8726

Conversation

pauldambra commented Feb 23, 2022 • edited Loading

Changes

How did you test this code?

mariusandra left a comment • edited Loading

Choose a reason for hiding this comment

pauldambra commented Feb 24, 2022

pauldambra commented Feb 27, 2022 • edited Loading

mariusandra left a comment • edited Loading

Choose a reason for hiding this comment

clarkus commented Mar 1, 2022

mariusandra commented Mar 3, 2022

pauldambra commented Mar 3, 2022

pauldambra commented Mar 4, 2022 • edited Loading

pauldambra commented Mar 4, 2022

mariusandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariusandra Mar 4, 2022 • edited Loading

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

pauldambra commented Mar 7, 2022

mariusandra left a comment

Choose a reason for hiding this comment

pauldambra commented Feb 23, 2022 •

edited

Loading

mariusandra left a comment •

edited

Loading

pauldambra commented Feb 27, 2022 •

edited

Loading

mariusandra left a comment •

edited

Loading

pauldambra commented Mar 4, 2022 •

edited

Loading

mariusandra Mar 4, 2022 •

edited

Loading