Entity picker/new search item component #42009

iethree · 2024-04-29T23:52:14Z

Description

Adds a new search results component that we will use for both recents and search. This implements it in search first so it can get into master while we work on some backend changes for recents.

When picking a collection (new > dashboard)

When picking a question (any dashboard > edit > hover a card > replace question)

Checklist

Tests have been added/updated to cover changes in this PR

iethree · 2024-04-30T16:54:47Z

enterprise/frontend/src/metabase-enterprise/collections/index.ts

+  PLUGIN_COLLECTIONS.getIcon = getIcon;
+


this should be part of verified collections rather than audit app

github-actions · 2024-04-30T16:55:03Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6065763...f9ac80d.

No notifications.

iethree · 2024-04-30T16:55:17Z

enterprise/frontend/src/metabase-enterprise/collections/utils.ts

@@ -59,7 +59,7 @@ export const getIcon = (item: ObjectWithModel): IconData => {
    };
  }

-  if (item.model === "dataset" && item.authority_level === "official") {
+  if (item.model === "dataset" && item.moderated_status === "verified") {


models are verified, collections are official 🥴

iethree · 2024-04-30T16:56:58Z

...tend/src/metabase/common/components/EntityPicker/components/ResultItem/ResultItem.styled.tsx

+  ${({ isLast }) =>
+    !isLast ? `border-bottom: 1px solid ${color("border")}` : ""};


unfortunately we need to do this programmatically, because the virtualizer adds some wrapping divs so we can't just check last-of-type in css 😢

iethree · 2024-04-30T16:58:04Z

frontend/src/metabase/common/components/EntityPicker/components/ResultItem/ResultItem.tsx

+                // required to get the tooltip to show up over the entity picker modal 😢
+                zIndex: "450 !important" as any,


I feel very bad about this, but the default zIndex for tooltips is 300, and the entity picker modal is at z-index 400 so it doesn't get overlapped by notifications that should be underneath it. Open to alternative ideas!

nit and proof that i'm a fuddy duddy, i've seen emojis break tooling in the past and prefer to avoid using them in code.

my only real feedback about the use of a very specific z-index # is that the context gets lots. i've previously used a constants file in the past that records all z-indexes so folks can understand the hierarchy in one file.

yeah, this is a result of a series of sins. I will

remove descriptions from this PR altogether so that this can merge

in a separate PR, hopefully tomorrow, fix the series of z-index sins in a separate PR

open a third PR to put descriptions back into this branch

Or can't you merge and just improve z-index sins in a follow up? That does sound like a lot only to avoid a magic number 🤔

hooray! i haven't had an opportunity to post my favorite meme lately.

iethree · 2024-04-30T17:00:32Z

...metabase/common/components/EntityPicker/components/EntityPickerSearch/EntityPickerSearch.tsx

                key={item.model + item.id}
-                result={Search.wrapEntity(item, dispatch)}


very happy that we no longer need redux involved here, just to get an icon

replay-io · 2024-04-30T17:06:48Z

Status	Complete ↗︎
Commit	`f9ac80d`
Results	⚠️ 9 Flaky ✅ 2456 Passed

sloansparger

Code looks good! I am noticing though that the UI flashes the loading spinner crazy fast before showing results. I think you could introduce a small delay before showing the spinner or show the stale results with a loading indicator elsewhere. Doing so would make this feel really polished! Great work!

...metabase/common/components/EntityPicker/components/EntityPickerSearch/EntityPickerSearch.tsx

...tend/src/metabase/common/components/EntityPicker/components/ResultItem/ResultItem.styled.tsx

frontend/src/metabase/common/components/EntityPicker/components/ResultItem/ResultItem.tsx

sloansparger · 2024-05-03T16:07:23Z

frontend/src/metabase/common/components/EntityPicker/components/ResultItem/ResultItem.tsx

+                // required to get the tooltip to show up over the entity picker modal 😢
+                zIndex: "450 !important" as any,


nit and proof that i'm a fuddy duddy, i've seen emojis break tooling in the past and prefer to avoid using them in code.

sloansparger · 2024-05-03T16:09:25Z

frontend/src/metabase/common/components/EntityPicker/components/ResultItem/ResultItem.tsx

+                // required to get the tooltip to show up over the entity picker modal 😢
+                zIndex: "450 !important" as any,


my only real feedback about the use of a very specific z-index # is that the context gets lots. i've previously used a constants file in the past that records all z-indexes so folks can understand the hierarchy in one file.

iethree · 2024-05-03T22:50:20Z

I am noticing though that the UI flashes the loading spinner crazy fast before showing results. I think you could introduce a small delay before showing the spinner or show the stale results with a loading indicator elsewhere. Doing so would make this feel really polished! Great work!

I updated this to use the delayed loading spinner, but I think the real problem is that the old search hook goes into loading mode and doesn't distinguish between refetching. I'd rather punt that to other planned work to use RTK query in this component, rather than enlarging the scope of this small PR.

github-actions · 2024-05-06T22:50:41Z

@iethree Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

kamilmielnik · 2024-05-07T07:51:01Z

frontend/src/metabase/lib/icon.ts

@@ -57,4 +59,9 @@ export const getIconBase = (
  return { name: modelIconMap?.[item.model] ?? "unknown" };
 };

-export const getIcon = PLUGIN_COLLECTIONS.getIcon ?? getIconBase;
+export const getIcon = (item: ObjectWithModel, options: IconOptions = {}) => {
+  if (PLUGIN_COLLECTIONS) {


Should we also take PLUGIN_MODERATION into account?

It also seems that getCollectionIcon has not been fully ported. Was this intentional?

metabase-bot bot assigned iethree Apr 29, 2024

metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label Apr 29, 2024

iethree changed the title ~~Entity picker/new search item component~~ [WIP] Entity picker/new search item component Apr 29, 2024

iethree added the no-backport Do not backport this PR to any branch label Apr 30, 2024

iethree changed the title ~~[WIP] Entity picker/new search item component~~ Entity picker/new search item component Apr 30, 2024

iethree marked this pull request as ready for review April 30, 2024 16:53

iethree force-pushed the entity-picker/new-search-item-component branch from 8fc53eb to cb642f7 Compare April 30, 2024 16:53

iethree added 3 commits April 30, 2024 10:54

add new entity picker result item

be7c6a7

simplify types

39b9ae0

enterprise icon fixes

56de155

iethree force-pushed the entity-picker/new-search-item-component branch from cb642f7 to 56de155 Compare April 30, 2024 16:54

iethree commented Apr 30, 2024

View reviewed changes

iethree requested a review from a team April 30, 2024 17:01

iethree added 2 commits April 30, 2024 11:22

unit test fixes

b03e8e8

design updates

c9bcbe6

iethree force-pushed the entity-picker/new-search-item-component branch from 2102cde to c9bcbe6 Compare May 2, 2024 22:24

sloansparger requested changes May 3, 2024

View reviewed changes

iethree added 2 commits May 3, 2024 16:31

address PR comments

56bc108

remove poorly implemented tooltip

7fbf839

iethree requested a review from sloansparger May 3, 2024 22:32

iethree added 2 commits May 3, 2024 17:00

fix unit test

2a2fd07

simplify spinner situation

f9ac80d

iethree force-pushed the entity-picker/new-search-item-component branch from b25b0f5 to f9ac80d Compare May 3, 2024 23:01

iethree mentioned this pull request May 6, 2024

Make Z-indexes a bit more sane #42272

Merged

1 task

sloansparger approved these changes May 6, 2024

View reviewed changes

iethree merged commit 0f8beaf into master May 6, 2024
110 checks passed

iethree deleted the entity-picker/new-search-item-component branch May 6, 2024 22:50

kamilmielnik reviewed May 7, 2024

View reviewed changes

iethree mentioned this pull request May 9, 2024

Entity Picker Recents Tab #39528

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entity picker/new search item component #42009

Entity picker/new search item component #42009

iethree commented Apr 29, 2024 •

edited

Loading

iethree Apr 30, 2024

github-actions bot commented Apr 30, 2024 •

edited

Loading

iethree Apr 30, 2024

iethree Apr 30, 2024

iethree Apr 30, 2024

sloansparger May 3, 2024

sloansparger May 3, 2024

iethree May 3, 2024

sloansparger May 3, 2024

iethree May 3, 2024

iethree May 6, 2024

iethree Apr 30, 2024

replay-io bot commented Apr 30, 2024 •

edited

Loading

sloansparger left a comment

sloansparger May 3, 2024

sloansparger May 3, 2024

iethree commented May 3, 2024

github-actions bot commented May 6, 2024

kamilmielnik May 7, 2024 •

edited

Loading

		${({ isLast }) =>
		!isLast ? `border-bottom: 1px solid ${color("border")}` : ""};

		// required to get the tooltip to show up over the entity picker modal 😢
		zIndex: "450 !important" as any,

		key={item.model + item.id}
		result={Search.wrapEntity(item, dispatch)}

Entity picker/new search item component #42009

Entity picker/new search item component #42009

Conversation

iethree commented Apr 29, 2024 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

github-actions bot commented Apr 30, 2024 • 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

replay-io bot commented Apr 30, 2024 • edited Loading

sloansparger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iethree commented May 3, 2024

github-actions bot commented May 6, 2024

kamilmielnik May 7, 2024 • edited Loading

Choose a reason for hiding this comment

iethree commented Apr 29, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

replay-io bot commented Apr 30, 2024 •

edited

Loading

kamilmielnik May 7, 2024 •

edited

Loading