add header search bar #81

frzifus · 2023-04-11T19:43:29Z

This pr is a poc for a search bar based on the fzf lib.

In addition to the listings of aws, gcp and azure images, another tab leading to a search is added. There you will find an unoptimized search, which renders all hits as links to the detail api point in the page.

Closes #75

deprecated details

Example:

During the implementation, I noticed that there is no modal for a detailed view of the images. This means that it is not possible to share specific images as a link. Does it make sense to add a route for it?

Another point is the search. Maybe it makes more sense to put the bar immediately in the "headline" and limit the suggestions (hits with the highest score) to 5-10 items. Similar to the google search.

wdyt @FKolwa @major @schogges @miyunari ?

frzifus

Well done @FKolwa! I like the new search performance 🥇

src/app/routes.tsx

schogges · 2023-04-14T23:18:50Z

src/app/components/Search.tsx

+    React.useEffect(() => {
+        loadData()
+    })


I guess loadData should only run when Search is getting mounted. Therefore I would recommend to add an empty dependency array, otherwise it would run on every render.

Suggested change

React.useEffect(() => {

loadData()

})

React.useEffect(() => {

loadData()

}, [])

Same for the effect in src/app/Pages/Browser/Search.tsx, not sure what will stay 🙂

The behavior was already discovered, but interesting fix 😄

I guess loadData should only run when Search is getting mounted. Therefore I would recommend to add an empty dependency array, otherwise it would run on every render.

Same for the effect in src/app/Pages/Browser/Search.tsx, not sure what will stay 🙂

Nice, great find! The sad thing is I ran into an issue on the image detail view in which the component stayed mounted after selecting a new image version which had the exact same fix (by monitoring the props passed in with the path) but I completely ignore this onEffect call.. face palm

src/app/components/Search.tsx

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

Replace fzf with fuzzysort for more accuracy and performance Highlight search results Print results to application launcher window

Add new dynamic routes for /image/<provider>/<region>/<imageName>

Co-authored-by: Mo <mf.89@gmx.de>

frzifus · 2023-04-15T08:21:55Z

@FKolwa @schogges do not wonder, I updated the branch and did some clean-up. 💅

One thing i noticed is, direct links do still not work.

frzifus · 2023-04-15T08:55:06Z

Weird https://localhost:9000/browser/google/global/rhel_9_arm64 does work, while https://localhost:9000/browser/google/global/rhel_8.2_sap_x86_64 does not. 🦝

FKolwa · 2023-04-15T10:24:21Z

One thing i noticed is, direct links do still not work. !

Yeah thats an issue for the WSGI server (the webpack server) running on your local machine. I think this is related to the missing 404 redirect in the development environment. The one page application is redirecting subdirectory calls to the router (or simpler, calls index.html) and renders the subpage accordingly. This works fine in our staging environment or in production but the webpack server does not (yet) know how to handle these calls. We should open an issue for that.

FKolwa · 2023-04-15T10:25:53Z

Weird https://localhost:9000/browser/google/global/rhel_9_arm64 does work, while https://localhost:9000/browser/google/global/rhel_8.2_sap_x86_64 does not. 🦝

Yeah I ran into the exact same thing. I don't know why some of them are properly redirect and others won't. In prod the CDN handles the proper redirect just fine, the dev server doesn't..

FKolwa

Only a little cleanup left and the missing package-lock changes need to be committed but everything else looks amazing! Thanks for doing this!

package.json

src/app/components/Search.tsx

FKolwa · 2023-04-15T12:00:38Z

src/app/routes.tsx

@@ -45,19 +47,25 @@ const routes: AppRouteConfig[] = [
    component: <AWSImageBrowser title='Cloud Image Directory | AWS Image Browser'/>,
    exact: true,
    label: 'AWS',
-    path: '/browser/AWS',
+    path: '/browser/aws',


Nice, thanks for fixing this.
This always annoyed me and I always forgot to fix it in the end

schogges

Well done everyone, this is working pretty smooth 🎉 👍 I just checked out the branch locally and have some more feedback 🙂

schogges · 2023-04-15T16:16:08Z

src/app/components/Search.tsx

+
+    return (
+        <>
+          {/* TODO: https://hy.reactjs.org/docs/hooks-reference.html#usecallback */}


This TODO seems to be misplaced now. I guess this was meant to be a TODO for handleChange 🤔 🙂

src/app/components/Search.tsx

src/app/Pages/ImageDetails.tsx

schogges

🚀

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

Co-authored-by: FKolwa <47077340+FKolwa@users.noreply.github.com>

Co-authored-by: Mo <mf.89@gmx.de>

frzifus · 2023-04-16T08:45:15Z

hm... seems there is still an issue with fetch in the unittests. if i understand it correctly, the problem is that fetch is not available. Which confuses me a bit. It should be available since v18.x that is used in the CI.

Anyway, I tried to install node-fetch. But that seems to expect types. Adding the type Map<string, number> as data response type in app/components/ImageDataTable.tsx fixed this, but when i tried to run webpack-dev afterwards, it seemed like hundreds of packages were missing.

For now, I have reverted the changes, what would you suggest to fix this? @FKolwa @schogges ? 🐎

https://stackoverflow.com/questions/48433783/referenceerror-fetch-is-not-defined

FKolwa · 2023-04-16T10:36:43Z

Anyway, I tried to install node-fetch. But that seems to expect types. Adding the type Map<string, number> as data response type in app/components/ImageDataTable.tsx fixed this, but when i tried to run webpack-dev afterwards, it seemed like hundreds of packages were missing.

Hmm. Okay so one problem is that fetch is a browser specific tool. As our jest tests aren't running in an actual browser fetch is simply not available. We didn't have this issue in our tests so far because they weren't testing anything where fetch was executed. Jest can run in different test environments, one of them is node. The node-fetch package was specifically made for enabling fetch in node. Our current test environment uses jsdom instead of node which at this moment doesn't support fetch (jestjs/jest#13834) .

So I'd say there are at least two ways of doing this: Further try to find a way around fetch in jsdom or try to rewrite our tests to use node instead (and require node-fetch in test-setup.js).

What do you think? @frzifus @schogges

schogges · 2023-04-16T11:40:39Z

Seems like even though they announced to remove the --experimental-fetch flag with release of v18, this was not the case.
I guess the easiest way to fix this now would be to use platform agnostic cross-fetch.
Another option for the future might be to use something like react-query as it ships some more features like caching, state values (isLoading, error, ...) and more 🙂

FKolwa · 2023-04-16T12:23:30Z

Seems like even though they announced to remove the --experimental-fetch flag with release of v18, this was not the case. I guess the easiest way to fix this now would be to use platform agnostic cross-fetch. Another option for the future might be to use something like react-query as it ships some more features like caching, state values (isLoading, error, ...) and more 🙂

I like it! I'd say we go with cross-fetch for now to keep within the scope of the issue and create a new issue to transition to react-query. That'll be a great replacement for the fetch calls we use to create the tables in our browser views.

frzifus · 2023-04-16T13:22:49Z

Ok, i will try to change it once iam home :)

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

for more information, see https://pre-commit.ci

FKolwa

@frzifus @schogges Amazing work everyone, thanks for the collaboration on this.
(created two follow up issues #83 and #84 )

src/app/components/Search.tsx

@schogges

Removed as suggested by @schogges

frzifus · 2023-04-17T06:55:50Z

oh i guess it would have been better to squash all this back and forth. 😵‍💫 😅

frzifus mentioned this pull request Apr 12, 2023

missing a way to copy links to a specific image #82

Closed

frzifus commented Apr 14, 2023

View reviewed changes

src/app/routes.tsx Outdated Show resolved Hide resolved

schogges reviewed Apr 14, 2023

View reviewed changes

src/app/components/Search.tsx Outdated Show resolved Hide resolved

frzifus and others added 4 commits April 15, 2023 10:13

search: init poc

6712c21

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

Add searchbar to header

5bfbd93

Replace fzf with fuzzysort for more accuracy and performance Highlight search results Print results to application launcher window

Add details view for search result

508b26c

Add new dynamic routes for /image/<provider>/<region>/<imageName>

search: avoid constant reloading of image list

42702d1

Co-authored-by: Mo <mf.89@gmx.de>

frzifus force-pushed the poc_search branch from 3c0cd6b to 92955d5 Compare April 15, 2023 08:15

frzifus requested review from schogges and FKolwa April 15, 2023 08:24

frzifus marked this pull request as ready for review April 15, 2023 08:25

frzifus changed the title ~~poc: search bar~~ add header search bar Apr 15, 2023

FKolwa approved these changes Apr 15, 2023

View reviewed changes

schogges reviewed Apr 15, 2023

View reviewed changes

frzifus force-pushed the poc_search branch from e92bc6c to 6d829db Compare April 15, 2023 19:14

schogges approved these changes Apr 15, 2023

View reviewed changes

frzifus and others added 5 commits April 16, 2023 10:10

search: remove unused code and other cleanup

9fd8a5a

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

routes: rename search routes

7ba4775

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

searchbar: remove console log

1fd58d8

Co-authored-by: FKolwa <47077340+FKolwa@users.noreply.github.com>

searchbar: prefer const over let

0164fe4

Co-authored-by: Mo <mf.89@gmx.de>

searchbar: use proper type to store state

9cec98b

Co-authored-by: Mo <mf.89@gmx.de>

frzifus force-pushed the poc_search branch 3 times, most recently from 18a2fff to 9cec98b Compare April 16, 2023 08:33

searchbar: use cross-fetch

31086cc

Signed-off-by: Benedikt Bongartz <bongartz@klimlive.de>

frzifus force-pushed the poc_search branch from af6e80e to 31086cc Compare April 16, 2023 22:45

[pre-commit.ci] auto fixes from pre-commit.com hooks

76c36e5

for more information, see https://pre-commit.ci

FKolwa approved these changes Apr 17, 2023

View reviewed changes

src/app/components/Search.tsx Outdated Show resolved Hide resolved

Update src/app/components/Search.tsx

435ae17

Removed as suggested by @schogges

FKolwa merged commit c88c76d into redhatcloudx:main Apr 17, 2023

This was referenced Apr 17, 2023

Webpack dev server cannot handle direct links to subdirectories #83

Closed

Migrate from fetch to react-query #84

Open

frzifus deleted the poc_search branch April 17, 2023 05:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add header search bar #81

add header search bar #81

frzifus commented Apr 11, 2023 •

edited

Loading

frzifus left a comment

schogges Apr 14, 2023 •

edited

Loading

frzifus Apr 14, 2023

FKolwa Apr 15, 2023

frzifus commented Apr 15, 2023

frzifus commented Apr 15, 2023

FKolwa commented Apr 15, 2023 •

edited

Loading

FKolwa commented Apr 15, 2023

FKolwa left a comment

FKolwa Apr 15, 2023

schogges left a comment

schogges Apr 15, 2023

schogges left a comment

frzifus commented Apr 16, 2023

FKolwa commented Apr 16, 2023 •

edited

Loading

schogges commented Apr 16, 2023 •

edited

Loading

FKolwa commented Apr 16, 2023

frzifus commented Apr 16, 2023

FKolwa left a comment •

edited

Loading

frzifus commented Apr 17, 2023

add header search bar #81

add header search bar #81

Conversation

frzifus commented Apr 11, 2023 • edited Loading

frzifus left a comment

Choose a reason for hiding this comment

schogges Apr 14, 2023 • edited Loading

Choose a reason for hiding this comment

frzifus Apr 14, 2023

Choose a reason for hiding this comment

FKolwa Apr 15, 2023

Choose a reason for hiding this comment

frzifus commented Apr 15, 2023

frzifus commented Apr 15, 2023

FKolwa commented Apr 15, 2023 • edited Loading

FKolwa commented Apr 15, 2023

FKolwa left a comment

Choose a reason for hiding this comment

FKolwa Apr 15, 2023

Choose a reason for hiding this comment

schogges left a comment

Choose a reason for hiding this comment

schogges Apr 15, 2023

Choose a reason for hiding this comment

schogges left a comment

Choose a reason for hiding this comment

frzifus commented Apr 16, 2023

FKolwa commented Apr 16, 2023 • edited Loading

schogges commented Apr 16, 2023 • edited Loading

FKolwa commented Apr 16, 2023

frzifus commented Apr 16, 2023

FKolwa left a comment • edited Loading

Choose a reason for hiding this comment

frzifus commented Apr 17, 2023

frzifus commented Apr 11, 2023 •

edited

Loading

schogges Apr 14, 2023 •

edited

Loading

FKolwa commented Apr 15, 2023 •

edited

Loading

FKolwa commented Apr 16, 2023 •

edited

Loading

schogges commented Apr 16, 2023 •

edited

Loading

FKolwa left a comment •

edited

Loading