nextstrain.org icon indicating copy to clipboard operation
nextstrain.org copied to clipboard

Port new resource-listing “cards” UI for use to display datasets on /groups and /community

Open genehack opened this issue 1 year ago • 10 comments

Work breakdown:

  • [x] update useDataFetch code to pull groups data when given appropriate arguments
  • [x] update useDataFetch code to transform groups data into expected shape
  • [x] update /groups page to use ResourceList component
  • [ ] ~convert narratives into simple list as suggested~
  • [x] add link from group name to group page
  • [ ] add custom logos when we have them
    • it doesn't appear we have these in any centralized way
    • is this worth doing per-group fetch() calls during the card render to hit the getSourceInfo endpoint to retrieve the avatar field and inject it over the default image? (feels expensive.)
    • "Then maybe best to extend metaSources to return group logos in addition to datasets/narratives" (context)
    • The way group logos are stored is really not conducive to accessing them in this way. I think what I'm going to do instead is repress the logo for anything that isn't nextstrain
  • [ ] add resource list to single-group pages
    • should be mostly lift and shift from /groups
  • [ ] add resource list to /community
  • [ ] add last-modified dates to group datasets
    • maybe/probably out of scope for this issue and better handled on its own?

genehack avatar May 20 '24 17:05 genehack

FYI I'm planning to make some edits to static-site/src/components/ListResources/Showcase.jsx proposed in https://github.com/nextstrain/nextstrain.org/issues/849#issuecomment-2121064824.

We may run into conflicts here, but I suspect that as long as you use the Showcase component just like this in the other pages, it should be trivial to resolve.

https://github.com/nextstrain/nextstrain.org/blob/61c0419da4dfb823ea081a65c2d4ad82b245b3cd/static-site/src/components/ListResources/index.jsx#L64

victorlin avatar May 20 '24 21:05 victorlin

@genehack happy to talk through any design questions you have here (as they arise). Perhaps a couple of points as to what I was thinking:

  • The current <ListResources> component gets its data from a hardcoded API call. The current groups/community pages uses the charon/getAvailable API. My suggestion is to allow the <ListResources> component to use those charon/getAvailable APIs, either by expanding useDataFetch or by injecting custom data fetch functions to the component.
  • Narratives aren't currently displayed in <ListResources>. They totally can be, but lots of UI decisions there. As an intermediate we could replace the existing narrative table with a simple HTML list, which I think would be just fine.

jameshadfield avatar May 20 '24 22:05 jameshadfield

@genehack happy to talk through any design questions you have here

Thanks, I was gonna use our 1:1 time today to get a bootstrapping brain dump from you on this. :grin:

genehack avatar May 21 '24 17:05 genehack

Noting that Heroku deployment is currently blocked on making eslint aware of TypeScript.

genehack avatar Jun 07 '24 03:06 genehack

Noting that Heroku deployment is currently blocked on https://github.com/nextstrain/nextstrain.org/issues/905.

I mean, we could just fix the linting errors highlighted by the failing build step (which you can run locally as well), which'll let this PR be built as a Heroku review app and even merged. Not a great long term state, but doesn't seem like something that should block this work.

jameshadfield avatar Jun 07 '24 04:06 jameshadfield

Noting that Heroku deployment is currently blocked on #905.

I mean, we could just fix the linting errors highlighted by the failing build step (which you can run locally as well), which'll let this PR be built as a Heroku review app and even merged. Not a great long term state, but doesn't seem like something that should block this work.

One of the linting errors is ESLint claiming the variable in a function type isn't being used — that is, the linting errors are because we're linting Typescript code with a linter that doesn't understand Typescript.

Screenshot 2024-06-07 at 11 19 24

I don't know how to fix that without fixing the linting — maybe I'm missing some angle?

genehack avatar Jun 07 '24 18:06 genehack

You can fix the warning by adding the dependency to useEffect and suppress the two errors arising from the lack of TypeScript parsing.

diff --git a/static-site/src/components/ListResources/index.tsx b/static-site/src/components/ListResources/index.tsx
index 467dec9d..d0443f97 100644
--- a/static-site/src/components/ListResources/index.tsx
+++ b/static-site/src/components/ListResources/index.tsx
@@ -123,6 +123,8 @@ interface ListResourcesResponsiveProps {
   defaultGroupLinks: boolean
   groupDisplayNames: GroupDisplayNames
   showcase: Card[]
+  // <https://github.com/nextstrain/nextstrain.org/issues/905>
+  // eslint-disable-next-line no-unused-vars
   parserCallBackFxn: (response: Response) => Promise<{ pathPrefix: string; pathVersions: PathVersions; }>;
 }
 
diff --git a/static-site/src/components/ListResources/useDataFetch.ts b/static-site/src/components/ListResources/useDataFetch.ts
index 49b9634e..372d30a9 100644
--- a/static-site/src/components/ListResources/useDataFetch.ts
+++ b/static-site/src/components/ListResources/useDataFetch.ts
@@ -20,6 +20,8 @@ export function useDataFetch(
   versioned: boolean,
   defaultGroupLinks: boolean,
   groupDisplayNames: GroupDisplayNames,
+  // <https://github.com/nextstrain/nextstrain.org/issues/905>
+  // eslint-disable-next-line no-unused-vars
   parserCallBack: (response: Response) => Promise<{ pathPrefix:string, pathVersions:PathVersions }>
 ) : {groups: Group[] | undefined, dataFetchError: boolean | undefined} {
   const [groups, setGroups] = useState<Group[]>();
@@ -66,7 +68,7 @@ export function useDataFetch(
     }
 
     fetchAndParse();
-  }, [sourceId, versioned, defaultGroupLinks, groupDisplayNames]);
+  }, [sourceId, versioned, defaultGroupLinks, groupDisplayNames, parserCallBack]);
   return {groups, dataFetchError}
 }
 

tsibley avatar Jun 07 '24 19:06 tsibley

You can fix the warning by adding the dependency to useEffect and suppress the two errors arising from the lack of TypeScript parsing.

Added the dep to useEffect; suppressing the two errors feels counter-productive, as I'll just have to take the disable statements back out once the linting fix lands.

genehack avatar Jun 07 '24 22:06 genehack

Canary version

genehack avatar Jun 14 '24 18:06 genehack

add custom logos when we have them

Slapping our logo next to all of these third-party Nextstrain Groups seems way less than ideal!

image

The static-site/components/list-resources/ UI hardcodes our logo, but should likely parameterize the logo per "resource group" instead:

https://github.com/nextstrain/nextstrain.org/blob/e1d369344528b33fa8cedc5f02f8aa4c5ee62ffb/static-site/components/list-resources/types.ts#L11-L21

Related to this, the resource listing for Nextclade dataset reference trees (https://github.com/nextstrain/nextstrain.org/pull/1249) shows the our logo next to Nextclade community datasets .

image

It'd be good to fix this logo issue sooner than later, even if the interim solution is a placeholder logo or nothing at all.

tsibley avatar Nov 04 '25 19:11 tsibley