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.

Sign up for GitHub

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

Jump to bottom

Update group cards further #928

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update group cards further #928

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f137aea
Set defaultGroupLinks to true [#870]
genehack Jun 21, 2024
875c352
Add `tooltipText` prop to ResourceGroupHeader [#870]
genehack Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions static-site/src/components/ListResources/ResourceGroup.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ interface ResourceGroupHeaderProps {
isCollapsed: boolean
resourcesToShowWhenCollapsed: number
quickLinks: QuickLink[]
tooltipText: string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are quite a few tooltips within the resources UI, so I'd consider a prop name such as defaultGroupLinksTooltip.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the entire text and gets added to later. I would call it something like groupTooltipPrefix.

}

const ResourceGroupHeader = ({group, isMobile, setCollapsed, collapsible, isCollapsed, resourcesToShowWhenCollapsed, quickLinks}: ResourceGroupHeaderProps) => {
const ResourceGroupHeader = ({group, isMobile, setCollapsed, collapsible, isCollapsed, resourcesToShowWhenCollapsed, quickLinks, tooltipText}: ResourceGroupHeaderProps) => {
if (!tooltipText) tooltipText="Click to load the default (and most recent) analysis";
const setModalResource = useContext(SetModalResourceContext);
Comment on lines +22 to 23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: don't set a default here. Make sure every usage of ListResources defines the right text based on the context.

if (!setModalResource) throw new Error("Context not provided!")

Expand All @@ -34,7 +36,7 @@ const ResourceGroupHeader = ({group, isMobile, setCollapsed, collapsible, isColl

<HeaderRow>
{group.groupUrl ? (
<TooltipWrapper description={`Click to load the default (and most recent) analysis for ${group.groupDisplayName || group.groupName}`}>
<TooltipWrapper description={`${tooltipText} for ${group.groupDisplayName || group.groupName}`}>
<GroupLink style={{ fontSize: '2rem', fontWeight: '500'}} href={group.groupUrl} target="_blank" rel="noreferrer">
{group.groupDisplayName || group.groupName}
</GroupLink>
Expand Down Expand Up @@ -118,12 +120,13 @@ interface ResourceGroupProps {
numGroups: number
sortMethod: string
quickLinks: QuickLink[]
tooltipText: string
}

/**
* Displays a single resource group (e.g. a single pathogen)
*/
export const ResourceGroup = ({group, elWidth, numGroups, sortMethod, quickLinks}: ResourceGroupProps) => {
export const ResourceGroup = ({group, elWidth, numGroups, sortMethod, quickLinks, tooltipText}: ResourceGroupProps) => {
const {collapseThreshold, resourcesToShowWhenCollapsed} = collapseThresolds(numGroups);
const collapsible = group.resources.length > collapseThreshold;
const [isCollapsed, setCollapsed] = useState(collapsible); // if it is collapsible, start collapsed
Expand All @@ -140,7 +143,7 @@ export const ResourceGroup = ({group, elWidth, numGroups, sortMethod, quickLinks
<ResourceGroupHeader group={group} quickLinks={quickLinks}
setCollapsed={setCollapsed} collapsible={collapsible}
isCollapsed={isCollapsed} resourcesToShowWhenCollapsed={resourcesToShowWhenCollapsed}
isMobile={isMobile}
isMobile={isMobile} tooltipText={tooltipText}
/>

<IndividualResourceContainer $maxResourceWidth={maxResourceWidth}>
Expand Down
3 changes: 3 additions & 0 deletions static-site/src/components/ListResources/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ function ListResources({
groupDisplayNames,
showcase,
resourceListingCallback: resourceListingCallback,
tooltipText,
}: ListResourcesProps) {
const {groups, dataFetchError} = useDataFetch(
versioned,
Expand Down Expand Up @@ -105,6 +106,7 @@ function ListResources({
elWidth={elWidth}
numGroups={resourceGroups.length}
sortMethod={sortMethod}
tooltipText={tooltipText}
/>
))}
</div>
Expand Down Expand Up @@ -133,6 +135,7 @@ interface ListResourcesResponsiveProps {
groupDisplayNames: Record<string, string>
showcase: FilterCard[]
resourceListingCallback: () => Promise<ResourceListingInfo>;
tooltipText: string
}

/**
Expand Down
6 changes: 5 additions & 1 deletion static-site/src/pages/groups.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,12 @@ class GroupsPage extends React.Component {

<ListResources
resourceType="dataset"
defaultGroupLinks={true}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[crystal ball gazing -- not a request for changes]

Implicit in the original design is that card titles will become links by simply adding the card name to the current URL. It works well for the groups page (e.g. groups + blab), but I don't think it'll work for an individual groups page.¹ We'll probably just disable the links for those pages.

Also, for what it's worth, the ={true} can be dropped, if you prefer.

¹ Depends on how card titles are chosen of course, but the blab group will probably have a "229e" card and /groups/blab/229e is 404.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends on how card titles are chosen of course, but the blab group will probably have a "229e" card and /groups/blab/229e is 404.

I would think on the individual groups page, the titles would not include groups/<group_name>. The resourceListingCallback would just return pathPrefix: "groups/<group_name>"? (Maybe I'm completely off here, still catching up on all the ListResources work)

versioned={false}
resourceListingCallback={resourceListingCallback}/>
resourceListingCallback={resourceListingCallback}
// FIXME I do not love this text string; suggestions welcome
tooltipText="Click to load the group page"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion]

"Click to view this group in isolation, including additional information specific to that group"

Won't look nice if you then tack on for ${group.groupDisplayName || group.groupName} but we could change the prop to be a function accepting the group name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Nextstrain docs for Groups call it a "splash page", so we can follow that pattern here:

"Click to view the individual group splash page"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or something less wordy:

Go to splash page for blab

Visit splash page for blab

/>

<HugeSpacer />

Expand Down