Improve the Dashboard Landing Page Experience #2615

jeff-phillips-18 · 2024-03-20T10:33:43Z

Closes: RHOAISTRAT-119

Description

Adds a new landing page.

Screenshots

How Has This Been Tested?

Test Impact

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

andrewballantyne · 2024-03-25T16:42:01Z

frontend/src/components/OdhDocCard.tsx

+  showFavorite,
+  favorite = false,
+  updateFavorite = () => {
+    // do nothing


TBD?

If you legit mean do nothing as a default... maybe use _.noop just so you don't need to have a comment.

Oh -- well don't add code that has no use 😛 I didn't reverse lookup usage lol

andrewballantyne · 2024-03-25T16:51:50Z

frontend/src/concepts/areas/types.ts

@@ -28,6 +28,8 @@ export enum SupportedArea {
  USER_MANAGEMENT = 'user-management',
  ACCELERATOR_PROFILES = 'accelerator-profiles',

+  HOME = 'home',


Nit, can we get this at the top of the enum?

andrewballantyne · 2024-03-25T17:03:17Z

frontend/src/k8sTypes.ts

@@ -1126,6 +1126,7 @@ export type DashboardCommonConfig = {
  disableProjects: boolean;
  disableModelServing: boolean;
  disableProjectSharing: boolean;
+  disableHome: boolean;


Ideally we should add this to the backend type too... we really need one shared type lol

andrewballantyne · 2024-03-25T17:06:50Z

frontend/src/pages/home/Home.tsx

+            <HintBody>
+              Your home page summarizes the projects you have access to, and includes helpful
+              resources such as quick starts.
+              <br />


Paragraph tags not a better use-case? It may be a personal feel, but I think if you're going to break a line, you should also have a gap.

andrewballantyne · 2024-03-25T17:08:06Z

frontend/src/pages/home/Home.tsx

+              <br />
+              The old{' '}
+              <Button
+                style={{ fontSize: 'initial' }}


Is this necessary? What is impacting the fontSize?

PF inline link buttons set the font size smaller. This overrides the setting.

Could you log a bug against PF if they cannot provide inline styles like that?

This seems like one of those "override it because it's wrong" and not "we want to do it because we want styles to be prescriptive" -- which has been the DS Projects & Home page goal here.

I would like to see the bug logged as a comment above weird edge-case code so we know when we can remove it

andrewballantyne · 2024-03-25T17:11:22Z

frontend/src/pages/home/OrganizeSection.tsx

+    <CardBody>
+      <Stack hasGutter>
+        <Bullseye>
+          <img height={32} src={imgSrc} alt="" />


We really shouldn't have no alt -- fairly certain that violates a11y.

It really depends on the usage of the image. If it's decorative and not meant to convey specific information, an empty alt will allow a decorative image to be hidden: https://www.w3.org/WAI/tutorials/images/decorative/

andrewballantyne · 2024-03-25T17:11:41Z

frontend/src/pages/home/OrganizeSection.tsx

+  onSelect: () => void;
+}
+
+const OrganizeCard: React.FC<OrganizeCardProps> = ({


Its own file please.

andrewballantyne · 2024-03-25T17:16:06Z

frontend/src/pages/home/OrganizeSection.tsx

+    <PageSection data-testid="landing-page-organize" variant="light" {...rest}>
+      <Stack hasGutter>
+        <TextContent>
+          <Text component="h1">


Unless this is the Home component, I don't imagine you should say h1 -- since it's not the top level control. Wouldn't this be h2 at the min?

We don't have a single title here so there is no main h1. a11y folks tell me multiple h1's are better than none.

Yeah, ideally we can have one h1 and then have subsequent h2s that are beneath the h1, to convey the information architecture of the page. However, we couldn't really tell what makes sense to have as the main heading of the page that all of the other headings are a part of. Ideally, we could add another heading that would be the h1, but I imagine that changes the design a bit.

andrewballantyne · 2024-03-25T17:35:07Z

frontend/src/pages/home/QuickStartsSection.tsx

+  const qsContext = React.useContext<QuickStartContextValues>(QuickStartContext);
+  const { components, loaded, loadError } = useWatchComponents(false);
+  const [shownQuickStarts, setShownQuickStarts] = React.useState<OdhDocument[]>([]);
+
+  React.useEffect(() => {
+    if (loaded && !loadError) {
+      const quickStarts =
+        qsContext.allQuickStarts?.slice(0, 4).map((quickStart) => {
+          const odhDoc = quickStart as unknown as OdhDocument;
+          const odhApp = components.find((c) => c.metadata.name === odhDoc.spec.appName);
+          const updatedDoc = {
+            ...odhDoc,
+            spec: { ...odhDoc.spec, type: OdhDocumentType.QuickStart },
+          };
+          if (odhApp) {
+            updatedDoc.spec.appDisplayName = odhApp.spec.displayName;
+            updatedDoc.spec.img = odhDoc.spec.img || odhApp.spec.img;
+            updatedDoc.spec.description = odhDoc.spec.description || odhApp.spec.description;
+          }
+          return updatedDoc;
+        }) ?? [];
+      setShownQuickStarts(quickStarts);
+    }
+  }, [loaded, loadError, qsContext.allQuickStarts, components]);


Dang, this is probably best not done by this component and you just return a list of items, then slice out you 5 -- you're trying to duplicate the wonky logic we have for Components -- which is currently on the backend (so not sharable code 😞).

Can we do a little organization and just make it a hook for "enabled components" or something that you steal the first 5 or filter and steal the first 5?

Also I believe this is a memo with extra steps -- not sure this async methodology is needed.

andrewballantyne · 2024-03-25T18:13:40Z

frontend/src/pages/home/OrganizeSection.tsx

+};
+
+const OrganizeSection: React.FC<OrganizeSectionProps> = ({ allowCreateProjects, ...rest }) => {
+  const { pipelinesServer } = usePipelinesAPI();


API is not available outside a project context... I think you mean to check the useIsAreaAvailable for Pipelines.

frontend/src/pages/home/Home.tsx

andrewballantyne · 2024-03-25T18:37:36Z

frontend/src/pages/home/Home.tsx

+
+const Home: React.FC = () => {
+  const navigate = useNavigate();
+  const [hintHidden, setHintHidden] = useLocalStorage('rhodsNewLandingPageMessage', null);


Can we call this odh.dashboard.<something> (eg. odh.dashboard.landing.message) or something to match with the other ODH storage flags we have?

openshift-ci · 2024-03-25T20:10:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from andrewballantyne. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-merge-robot · 2024-04-07T13:59:23Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

andrewballantyne

Temp review -- we will break down the PR, so this is not needed changes with this PR.

andrewballantyne · 2024-04-16T12:54:27Z

frontend/src/app/AppRoutes.tsx

+        <Route path="/" element={<HomePage />} />
+        <Route path="/applications" element={<InstalledApplications />} />


You have a feature flag but it is not going to be used to redirect the home back to InstalledApplications?

andrewballantyne · 2024-04-16T12:57:09Z

frontend/src/concepts/design/DividedGallery.tsx

-          <Button aria-label="close" isInline variant="plain" onClick={onClose}>
-            <TimesIcon />
+          <Button aria-label={closeAlt || 'close'} isInline variant="plain" onClick={onClose}>
+            <TimesIcon alt={`close ${closeAlt}`} />


Shouldn't you default closeAlt to close in the props? Wouldn't this just be close if you omit the property?

andrewballantyne · 2024-04-16T13:10:18Z

frontend/src/pages/home/Home.tsx

+              <br />
+              The old{' '}
+              <Button
+                style={{ fontSize: 'initial' }}


I would like to see the bug logged as a comment above weird edge-case code so we know when we can remove it

andrewballantyne · 2024-04-16T13:11:26Z

frontend/src/pages/home/Home.tsx

+
+  return (
+    <>
+      {!hintHidden ? (


can we move this whole hint block into another component? It has a lot of text and nuance that really doesn't belong to the Home page component.

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Mar 20, 2024

openshift-ci bot requested review from DaoDaoNoCode and manaswinidas March 20, 2024 10:33

jeff-phillips-18 force-pushed the home-page branch 8 times, most recently from 327c323 to 7df36d9 Compare March 20, 2024 21:14

openshift-merge-robot added the needs-rebase PR needs to be rebased label Mar 22, 2024

jeff-phillips-18 force-pushed the home-page branch from 7df36d9 to 824f816 Compare March 22, 2024 13:09

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Mar 22, 2024

jeff-phillips-18 force-pushed the home-page branch 6 times, most recently from 45b7464 to 2bad9ba Compare March 25, 2024 14:37

jeff-phillips-18 changed the title ~~[WIP] Improve the Dashboard Landing Page Experience~~ Improve the Dashboard Landing Page Experience Mar 25, 2024

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Mar 25, 2024

andrewballantyne requested changes Mar 25, 2024

View reviewed changes

openshift-ci bot assigned andrewballantyne Mar 25, 2024

andrewballantyne reviewed Mar 25, 2024

View reviewed changes

frontend/src/pages/home/Home.tsx Show resolved Hide resolved

andrewballantyne reviewed Mar 25, 2024

View reviewed changes

Add home page

8ab998f

jeff-phillips-18 force-pushed the home-page branch from 2bad9ba to 8ab998f Compare March 25, 2024 20:10

openshift-merge-robot added the needs-rebase PR needs to be rebased label Apr 7, 2024

andrewballantyne reviewed Apr 16, 2024

View reviewed changes

jeff-phillips-18 closed this Apr 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve the Dashboard Landing Page Experience #2615

Improve the Dashboard Landing Page Experience #2615

jeff-phillips-18 commented Mar 20, 2024 •

edited

andrewballantyne Mar 25, 2024

andrewballantyne Mar 25, 2024 •

edited

andrewballantyne Mar 25, 2024

andrewballantyne Mar 25, 2024

andrewballantyne Mar 25, 2024

andrewballantyne Mar 25, 2024

jeff-phillips-18 Mar 25, 2024

andrewballantyne Mar 25, 2024

andrewballantyne Apr 16, 2024

andrewballantyne Mar 25, 2024

jessiehuff Mar 25, 2024

andrewballantyne Mar 25, 2024

andrewballantyne Mar 25, 2024

jeff-phillips-18 Mar 25, 2024

jessiehuff Mar 25, 2024

andrewballantyne Mar 25, 2024

andrewballantyne Mar 25, 2024

andrewballantyne Mar 25, 2024

openshift-ci bot commented Mar 25, 2024

openshift-merge-robot commented Apr 7, 2024

andrewballantyne left a comment

andrewballantyne Apr 16, 2024

andrewballantyne Apr 16, 2024

andrewballantyne Apr 16, 2024

andrewballantyne Apr 16, 2024

		<Route path="/" element={<HomePage />} />
		<Route path="/applications" element={<InstalledApplications />} />

Improve the Dashboard Landing Page Experience #2615

Improve the Dashboard Landing Page Experience #2615

Conversation

jeff-phillips-18 commented Mar 20, 2024 • edited

Description

Screenshots

How Has This Been Tested?

Test Impact

Request review criteria:

Choose a reason for hiding this comment

andrewballantyne Mar 25, 2024 • edited

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

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

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

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

Choose a reason for hiding this comment

openshift-ci bot commented Mar 25, 2024

openshift-merge-robot commented Apr 7, 2024

andrewballantyne left a comment

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

Choose a reason for hiding this comment

jeff-phillips-18 commented Mar 20, 2024 •

edited

andrewballantyne Mar 25, 2024 •

edited