[search ui] Display asset search on asset landing page #20373

clairelin135 · 2024-03-08T21:59:56Z

Walkthrough of the UI: https://www.loom.com/share/182595e3ba40415694343c6d786720d7?sid=ecb71bf9-0a87-44e4-bb78-4673d6d06495

Changes included:

Allows the SearchDialog to accept a prop indicating whether the search displays as an overlay, or on the page
Displays the fuzzy matches in the label in bold
Displays filter results, alongside the # of assets

Next steps:

Handle typeaheads (i.e. asset is:/sensor is:/etc)

clairelin135 · 2024-03-08T22:00:07Z

[search ui] Display asset search on asset landing page #20373 👈
[search ui] Add asset filter fields to search index #20372
[ui] Enable filtering asset table by owner #20420
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @clairelin135 and the rest of your teammates on Graphite

github-actions · 2024-03-08T22:02:50Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-dj5bmshpf-elementl.vercel.app
https://03-07-claire-styling-display-search-on-asset-catalog-page.core-storybook.dagster-docs.io

Built with commit 457bb25.
This pull request is being automatically deployed with vercel-action

hellendag

I think the approach of reusing SearchDialog with a separate render branch for each use case isn't quite right.

I think instead, SearchDialog and this new UI should share common components.

hellendag · 2024-03-11T14:11:20Z

js_modules/dagster-ui/packages/ui-core/src/search/SearchDialog.tsx

  isAssetSearch: boolean;
+  displayAsOverlay?: boolean;


The addition of isAssetSearch is an indication that this isn't really the right component for this UI to use. I see that that's from another branch, so I'll take a look at that one.

hellendag · 2024-03-11T14:14:44Z

js_modules/dagster-ui/packages/ui-core/src/search/SearchDialog.tsx

  );
+
+  if (displayAsOverlay) {


I don't think reusing SearchDialog with overriding props is quite the right approach. There are some similarities between global search and this UI, but I think the way to approach this is to have separate components that share common components and hooks.

That is, I would suggest that SearchDialog be left effectively as-is, without additional props or rendering branches, and some of its pieces could be factored out to be used by both (existing) global search and a new asset search component.

clairelin135 · 2024-03-11T19:19:14Z

js_modules/dagster-ui/packages/ui-core/src/search/AssetSearch.tsx

+      searchAndHandleSecondary(queryString);
+    }, DEBOUNCE_MSEC);
+    return (queryString: string) => {
+      if (!firstSearchTrace.current && isFirstSearch.current) {


not sure what this trace logic is, but it existed in the global search dialog so I added similar logic here?

clairelin135 · 2024-03-11T19:27:35Z

@hellendag thanks for the feedback and the explanation here. I agree that it's cleaner to make asset search live in its own component. I added this refactor and I think this logic is looking better :)

I wasn't entirely sure whether callbacks that mutate state should exist as shared functions or not, but aired on not sharing to make the state transitions per component clearer, though this does create some duplicate logic.

The new changes include:

Asset search now no longer executes the primary search query
Asset search now has a separate reducer, since it only uses a subset of the actions in global search.
Subsequently a copy of the dispatch callbacks to update search results and handle key events exist in SearchDialog and AssetSearch, since their behavior is slightly different (the global search version also handles closing the dialog)

Let me know how this looks.

salazarm · 2024-03-12T05:43:26Z

js_modules/dagster-ui/packages/ui-core/src/search/AssetSearch.tsx

+    return (queryString: string) => {
+      if (!firstSearchTrace.current && isFirstSearch.current) {
+        isFirstSearch.current = false;
+        const trace = createTrace('SearchDialog:FirstSearch');


Can you rename this to AssetSearch:FirstSearch

salazarm · 2024-03-12T05:46:46Z

js_modules/dagster-ui/packages/ui-core/src/search/SearchResults.tsx

+  font-weight: 500;
+`;
+
+const SearchResultBoldedLabel = styled.span`
+  font-weight: 800;
+`;
+
+const SearchResultLabel = styled.span`
+  font-weight: 300;
+`;


Generally you should not need to set font-weight in CSS. All of the styles used in figma should be defined here

dagster/js_modules/dagster-ui/packages/ui-components/src/components/Text.tsx

Line 35 in f7d2611

export const Subheading = styled(Text)`

The figma should specify whether it's Body2, Subheading2, etc.

makes sense, updated

github-actions · 2024-03-12T19:13:51Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-gm5mz9leo-elementl.vercel.app
https://03-07-claire-styling-display-search-on-asset-catalog-page.components-storybook.dagster-docs.io

Built with commit 457bb25.
This pull request is being automatically deployed with vercel-action

salazarm · 2024-03-12T19:44:05Z

js_modules/dagster-ui/packages/ui-components/src/components/Text.tsx

+export const CaptionBolded = styled(Text)`
+  font-family: ${FontFamily.default};
+  font-size: 12px;
+  font-weight: 900;


I assume this comes from figma?

salazarm · 2024-03-12T19:49:01Z

js_modules/dagster-ui/packages/ui-core/src/search/SearchResults.tsx

-          <Label isHighlight={isHighlight}>{item.label}</Label>
-          <Description isHighlight={isHighlight}>{item.description}</Description>
-        </div>
+        <Box flex={{direction: 'row', alignItems: 'center'}} style={{width: '100%'}}>


i think you want grow: 1 instead of width: 100%

salazarm · 2024-03-12T19:49:40Z

js_modules/dagster-ui/packages/ui-core/src/search/useGlobalSearch.tsx

@@ -249,6 +249,7 @@ const fuseOptions = {
  keys: ['label', 'segments', 'tags', 'type'],
  threshold: 0.3,
  useExtendedSearch: true,
+  includeMatches: true,


Feedback addressed

Walkthrough of the UI: https://www.loom.com/share/182595e3ba40415694343c6d786720d7?sid=ecb71bf9-0a87-44e4-bb78-4673d6d06495 Changes included: - Allows the `SearchDialog` to accept a prop indicating whether the search displays as an overlay, or on the page - Displays the fuzzy matches in the label in bold - Displays filter results, alongside the # of assets Next steps: - Handle typeaheads (i.e. `asset is:`/`sensor is:`/etc)

clairelin135 mentioned this pull request Mar 8, 2024

[search ui] Add asset filter fields to search index #20372

Merged

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from c5d9429 to 9ce9f77 Compare March 8, 2024 22:05

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch 3 times, most recently from efb1513 to 8755137 Compare March 9, 2024 00:13

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch 2 times, most recently from 445969c to 104a5dd Compare March 9, 2024 01:04

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch from 8755137 to 9de78a1 Compare March 9, 2024 01:06

clairelin135 changed the base branch from 03-07-claire/asset-search-ui to master March 9, 2024 01:08

clairelin135 changed the base branch from master to 03-07-claire/asset-search-ui March 9, 2024 01:08

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch 2 times, most recently from 1bdc7f9 to e477c43 Compare March 9, 2024 01:23

clairelin135 marked this pull request as ready for review March 9, 2024 01:24

clairelin135 requested review from salazarm, hellendag and bengotow March 9, 2024 01:25

hellendag previously requested changes Mar 11, 2024

View reviewed changes

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from 104a5dd to 2c06f1d Compare March 11, 2024 19:08

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch 2 times, most recently from b6ce9a5 to 0fec26d Compare March 11, 2024 19:13

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from 2c06f1d to 1010eea Compare March 11, 2024 19:15

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch from 0fec26d to 4c23fcf Compare March 11, 2024 19:15

clairelin135 commented Mar 11, 2024

View reviewed changes

clairelin135 requested a review from hellendag March 11, 2024 19:27

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from 1010eea to ecffd16 Compare March 11, 2024 19:41

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch from 4c23fcf to 5f2434c Compare March 11, 2024 19:42

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from ecffd16 to ce41857 Compare March 11, 2024 21:35

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch from 8ea6b1f to eeea70d Compare March 11, 2024 23:38

salazarm reviewed Mar 12, 2024

View reviewed changes

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch 2 times, most recently from c92d186 to 1cdabc9 Compare March 12, 2024 18:53

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch from eeea70d to e30c186 Compare March 12, 2024 18:53

clairelin135 requested a review from salazarm March 12, 2024 19:11

salazarm reviewed Mar 12, 2024

View reviewed changes

salazarm approved these changes Mar 12, 2024

View reviewed changes

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from 1cdabc9 to 289c094 Compare March 12, 2024 20:10

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch 2 times, most recently from 49feb08 to 3c7a44f Compare March 12, 2024 20:18

Base automatically changed from 03-07-claire/asset-search-ui to master March 12, 2024 21:31

clairelin135 added 8 commits March 12, 2024 14:31

claire/styling-display-search-on-asset-catalog-page

12e7d8d

display filter results & bold matches

991601f

add different styling for filter results

0ff1beb

display filter results & bold matches

764276a

add different styling for filter results

6608100

refactor asset search to be in separate component

d4da341

initialize upon load

47bfc9f

add font styling to text file

457bb25

clairelin135 force-pushed the 03-07-claire/styling-display-search-on-asset-catalog-page branch from 3c7a44f to 457bb25 Compare March 12, 2024 21:32

clairelin135 merged commit dd8fb52 into master Mar 12, 2024
3 checks passed

clairelin135 deleted the 03-07-claire/styling-display-search-on-asset-catalog-page branch March 12, 2024 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[search ui] Display asset search on asset landing page #20373

[search ui] Display asset search on asset landing page #20373

clairelin135 commented Mar 8, 2024 •

edited

Loading

clairelin135 commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

hellendag left a comment

hellendag Mar 11, 2024

hellendag Mar 11, 2024 •

edited

Loading

clairelin135 Mar 11, 2024

clairelin135 commented Mar 11, 2024 •

edited

Loading

salazarm Mar 12, 2024

clairelin135 Mar 12, 2024

salazarm Mar 12, 2024

clairelin135 Mar 12, 2024

github-actions bot commented Mar 12, 2024 •

edited

Loading

salazarm Mar 12, 2024

clairelin135 Mar 12, 2024

salazarm Mar 12, 2024

salazarm Mar 12, 2024

[search ui] Display asset search on asset landing page #20373

[search ui] Display asset search on asset landing page #20373

Conversation

clairelin135 commented Mar 8, 2024 • edited Loading

clairelin135 commented Mar 8, 2024 • edited Loading

github-actions bot commented Mar 8, 2024 • edited Loading

hellendag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellendag Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairelin135 commented Mar 11, 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

github-actions bot commented Mar 12, 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

clairelin135 commented Mar 8, 2024 •

edited

Loading

clairelin135 commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

hellendag Mar 11, 2024 •

edited

Loading

clairelin135 commented Mar 11, 2024 •

edited

Loading

github-actions bot commented Mar 12, 2024 •

edited

Loading