Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
4 of 9 tasks
Tracked by #947
genehack opened this issue May 20, 2024 · 9 comments
Open
4 of 9 tasks
Tracked by #947
Assignees
Labels
enhancement New feature or request

Comments

@genehack
Copy link
Contributor

genehack commented May 20, 2024

Work breakdown:

  • update useDataFetch code to pull groups data when given appropriate arguments
  • update useDataFetch code to transform groups data into expected shape
  • update /groups page to use ResourceList component
  • convert narratives into simple list as suggested
  • 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 genehack added the enhancement New feature or request label May 20, 2024
@genehack genehack self-assigned this May 20, 2024
@victorlin
Copy link
Member

FYI I'm planning to make some edits to static-site/src/components/ListResources/Showcase.jsx proposed in #849 (comment).

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.

<Showcase cards={showcaseCards} setSelectedFilterOptions={setSelectedFilterOptions}/>

@jameshadfield
Copy link
Member

@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.

@genehack
Copy link
Contributor Author

@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. 😁

genehack added a commit that referenced this issue Jun 6, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 6, 2024
* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 6, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
@genehack
Copy link
Contributor Author

genehack commented Jun 7, 2024

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

@jameshadfield
Copy link
Member

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.

@genehack
Copy link
Contributor Author

genehack commented Jun 7, 2024

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?

@tsibley
Copy link
Member

tsibley commented Jun 7, 2024

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}
 }
 

genehack added a commit that referenced this issue Jun 7, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 7, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 7, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 7, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
@genehack
Copy link
Contributor Author

genehack commented Jun 7, 2024

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 added a commit that referenced this issue Jun 7, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 7, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 10, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 10, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 10, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 10, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 10, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 10, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 10, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 10, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 10, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 12, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 12, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 12, 2024
…urces [#870]

* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with custom useDataFetch callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 12, 2024
genehack added a commit that referenced this issue Jun 12, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 12, 2024
…urces [#870]

* make the Resource.lastUpdated property optional
* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with ListResources callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 12, 2024
genehack added a commit that referenced this issue Jun 12, 2024
out of scope for the main thrust of the branch, but quiets the
typechecker.
genehack added a commit that referenced this issue Jun 12, 2024
…urces [#870]

* make the Resource.lastUpdated property optional
* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with ListResources callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 12, 2024
genehack added a commit that referenced this issue Jun 12, 2024
out of scope for the main thrust of the branch, but quiets the
typechecker.
genehack added a commit that referenced this issue Jun 13, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 13, 2024
…urces [#870]

* make the Resource.lastUpdated property optional
* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with ListResources callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 13, 2024
genehack added a commit that referenced this issue Jun 13, 2024
out of scope for the main thrust of the branch, but quiets the
typechecker.
genehack added a commit that referenced this issue Jun 13, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 13, 2024
…urces [#870]

* make the Resource.lastUpdated property optional
* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with ListResources callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 13, 2024
genehack added a commit that referenced this issue Jun 13, 2024
out of scope for the main thrust of the branch, but quiets the
typechecker.
genehack added a commit that referenced this issue Jun 13, 2024
Convert the two existing uses into the callback form.

This does not add any new functionality, it only re-arranges
implementation of existing functionality.
genehack added a commit that referenced this issue Jun 13, 2024
…urces [#870]

* make the Resource.lastUpdated property optional
* conditionalize display of "List known update" in IndividualResource
  TooltipWrapper
* conditionalize display of "Most recent snapshot" in ResourceGroup
* conditionalize use of Showcase in ListResources based on length of
  showcaseCards prop
* conditionalize use of SortOptions in ListResources based on initial
  group lacking a lastUpdated field
* extend groups page with ListResources callback to hit
  charon/getAvailableResources endpoint
genehack added a commit that referenced this issue Jun 13, 2024
genehack added a commit that referenced this issue Jun 13, 2024
out of scope for the main thrust of the branch, but quiets the
typechecker.
genehack added a commit that referenced this issue Jun 14, 2024
Replace Datasets DatasetSelect component on groups page with ListResources [#870]
@genehack
Copy link
Contributor Author

Canary version

genehack added a commit that referenced this issue Jun 21, 2024
This causes the group name to be rendered as a link to the group page.

This still uses the default tooltip, which is (IMO) wrong for the
links on the `/groups` page.
genehack added a commit that referenced this issue Jun 21, 2024
* If not provided, defaults to the former hard-coded tooltip value
* Includes changes to all `tooltipText` to be provided at to the
  `ListResources` component, as that's the level that knows what the
  tooltip for all the card should be

As noted in the `FIXME` comment: I'm not wild about the text I came up
with for the groups page and would welcome a better phrasing.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants