Resolve majority of remaining issues (opendatahub-io#38)

Resolve majority of low and remaining ui issues Additional updates from feedback Fix lint errors Undo reference change Undo hardcoded alert variant Update help button styling per feedback
thatblindgeye · Nov 22, 2024 · c94f738 · c94f738
1 parent 20cea58
commit c94f738
Show file tree

Hide file tree

Showing 57 changed files with 278 additions and 318 deletions.
diff --git a/frontend/src/app/HeaderTools.tsx b/frontend/src/app/HeaderTools.tsx
@@ -16,12 +16,7 @@ import {
   ToggleGroupItem,
   Icon,
 } from '@patternfly/react-core';
-import {
-  ExternalLinkAltIcon,
-  QuestionCircleIcon,
-  MoonIcon,
-  SunIcon,
-} from '@patternfly/react-icons';
+import { QuestionCircleIcon, MoonIcon, SunIcon } from '@patternfly/react-icons';
 import { COMMUNITY_LINK, DOC_LINK, SUPPORT_LINK, DEV_MODE, EXT_CLUSTER } from '~/utilities/const';
 import useNotification from '~/utilities/useNotification';
 import { updateImpersonateSettings } from '~/services/impersonateService';
@@ -104,8 +99,9 @@ const HeaderTools: React.FC<HeaderToolsProps> = ({ onNotificationsClick }) => {
         to={DOC_LINK}
         target="_blank"
         rel="noopener noreferrer"
+        isExternalLink
       >
-        Documentation <ExternalLinkAltIcon />
+        Documentation
       </DropdownItem>,
     );
   }
@@ -117,8 +113,9 @@ const HeaderTools: React.FC<HeaderToolsProps> = ({ onNotificationsClick }) => {
         to={SUPPORT_LINK}
         target="_blank"
         rel="noopener noreferrer"
+        isExternalLink
       >
-        Support <ExternalLinkAltIcon />
+        Support
       </DropdownItem>,
     );
   }
@@ -130,8 +127,9 @@ const HeaderTools: React.FC<HeaderToolsProps> = ({ onNotificationsClick }) => {
         to={COMMUNITY_LINK}
         target="_blank"
         rel="noopener noreferrer"
+        isExternalLink
       >
-        Community <ExternalLinkAltIcon />
+        Community
       </DropdownItem>,
     );
   }
@@ -153,35 +151,6 @@ const HeaderTools: React.FC<HeaderToolsProps> = ({ onNotificationsClick }) => {
     <Toolbar isFullHeight>
       <ToolbarContent>
         <ToolbarGroup variant="action-group-plain" align={{ default: 'alignEnd' }}>
-          <ToolbarItem>
-            <ToggleGroup aria-label="Dark theme toggle group">
-              <ToggleGroupItem
-                aria-label="light theme toggle"
-                icon={
-                  <Icon size="md">
-                    <SunIcon />
-                  </Icon>
-                }
-                isSelected={!isDarkTheme}
-                onChange={() => setIsDarkTheme(false)}
-              />
-              <ToggleGroupItem
-                aria-label="dark theme toggle"
-                icon={
-                  <Icon size="md">
-                    <MoonIcon />
-                  </Icon>
-                }
-                isSelected={isDarkTheme}
-                onChange={() => setIsDarkTheme(true)}
-              />
-            </ToggleGroup>
-          </ToolbarItem>
-          {!dashboardConfig.spec.dashboardConfig.disableAppLauncher ? (
-            <ToolbarItem data-testid="application-launcher">
-              <AppLauncher />
-            </ToolbarItem>
-          ) : null}
           <ToolbarItem>
             <NotificationBadge
               aria-label="Notification drawer"
@@ -190,6 +159,11 @@ const HeaderTools: React.FC<HeaderToolsProps> = ({ onNotificationsClick }) => {
               onClick={onNotificationsClick}
             />
           </ToolbarItem>
+          {!dashboardConfig.spec.dashboardConfig.disableAppLauncher ? (
+            <ToolbarItem data-testid="application-launcher">
+              <AppLauncher />
+            </ToolbarItem>
+          ) : null}
           <ToolbarItem>
             <Dropdown
               popperProps={{ position: 'right' }}
@@ -211,6 +185,30 @@ const HeaderTools: React.FC<HeaderToolsProps> = ({ onNotificationsClick }) => {
               <DropdownList>{helpMenuItems}</DropdownList>
             </Dropdown>
           </ToolbarItem>
+          <ToolbarItem>
+            <ToggleGroup aria-label="Theme toggle group">
+              <ToggleGroupItem
+                aria-label="light theme"
+                icon={
+                  <Icon size="md">
+                    <SunIcon />
+                  </Icon>
+                }
+                isSelected={!isDarkTheme}
+                onChange={() => setIsDarkTheme(false)}
+              />
+              <ToggleGroupItem
+                aria-label="dark theme"
+                icon={
+                  <Icon size="md">
+                    <MoonIcon />
+                  </Icon>
+                }
+                isSelected={isDarkTheme}
+                onChange={() => setIsDarkTheme(true)}
+              />
+            </ToggleGroup>
+          </ToolbarItem>
         </ToolbarGroup>
         {DEV_MODE && isImpersonating && (
           <ToolbarItem>
@@ -236,7 +234,6 @@ const HeaderTools: React.FC<HeaderToolsProps> = ({ onNotificationsClick }) => {
             onOpenChange={(isOpen) => setUserMenuOpen(isOpen)}
             toggle={(toggleRef: React.Ref<MenuToggleElement>) => (
               <MenuToggle
-                variant="plainText"
                 aria-label="User menu"
                 id="user-menu-toggle"
                 ref={toggleRef}

diff --git a/frontend/src/components/BrandImage.tsx b/frontend/src/components/BrandImage.tsx
@@ -34,6 +34,7 @@ const BrandImage: React.FC<BrandImageProps> = ({ src, ...props }) => {
   return (
     <Brand
       {...props}
+      className="odh-brand"
       heights={{ default: '40px' }}
       widths={{ default: '100%' }}
       onError={() => setImage((prevImage) => ({ imgSrc: prevImage.imgSrc, isValid: false }))}

diff --git a/frontend/src/components/DashboardDescriptionListGroup.tsx b/frontend/src/components/DashboardDescriptionListGroup.tsx
@@ -100,7 +100,6 @@ const DashboardDescriptionListGroup: React.FC<DashboardDescriptionListGroupProps
                   <Button
                     data-testid={editButtonTestId}
                     aria-label={`Edit ${title}`}
-                    isInline
                     variant="link"
                     icon={<PencilAltIcon />}
                     iconPosition="start"

diff --git a/frontend/src/components/ExternalLink.tsx b/frontend/src/components/ExternalLink.tsx
@@ -17,7 +17,7 @@ const ExternalLink: React.FC<ExternalLinkProps> = ({ text, to }) => (
       fireLinkTrackingEvent('ExternalLink Clicked', { href: to, from: window.location.pathname });
     }}
     icon={<ExternalLinkAltIcon />}
-    iconPosition="right"
+    iconPosition="end"
   >
     {text}
   </Button>

diff --git a/frontend/src/components/FavoriteButton.scss b/frontend/src/components/FavoriteButton.scss
@@ -1,19 +1,11 @@
 // This is an override provided by UX to set the star icon styles
 // Because this is not implemented in PF Card component
 .odh-favorite-button {
-  // Note from PatternFly: A choice should be made between using no padding on these buttons (now applying our pf-m-no-padding class to the button),
-  // or removing the below hover styles (a PF v6 button with padding would have hover styles by default).
-  // The contrast of the star-icon when selected in light theme may be difficult to see
   &__star-icon {
     fill: var(--pf-t--global--icon--color--subtle);
-    &:hover {
-      fill: var(--pf-t--global--icon--color--regular);
-    }
+
     &--is-selected {
       fill: var(--pf-t--global--icon--color--favorite--default);
-      &:hover {
-        fill: var(--pf-t--global--icon--color--favorite--hover);
-      }
     }
   }
 }
diff --git a/frontend/src/components/FavoriteButton.tsx b/frontend/src/components/FavoriteButton.tsx
@@ -20,7 +20,6 @@ const FavoriteButton: React.FC<FavoriteButtonProps> = ({ isFavorite, onClick })
       />
     }
     variant="plain"
-    hasNoPadding
     aria-label={isFavorite ? 'starred' : 'not starred'}
     className="odh-favorite-button"
     onClick={onClick}

diff --git a/frontend/src/components/GenericSidebar.tsx b/frontend/src/components/GenericSidebar.tsx
@@ -24,12 +24,17 @@ const GenericSidebar: React.FC<GenericSidebarProps> = ({
   maxWidth,
 }) => (
   <Sidebar hasGutter>
-    <SidebarPanel variant="sticky" style={{ maxWidth, top: 'var(--pf-t--global--spacer--md)' }}>
+    {/* Note from PF: the zIndex override here can be removed once the following issue is resolved:
+    https://github.com/patternfly/patternfly/issues/7229
+    */}
+    <SidebarPanel variant="sticky" style={{ maxWidth, zIndex: 'var(--pf-t--global--z-index--sm)' }}>
       <JumpLinks
         isVertical
         label="Jump to section"
         scrollableSelector={scrollableSelector}
         offset={16}
+        expandable={{ default: 'expandable', md: 'nonExpandable' }}
+        isExpanded
       >
         {sections.map((section) => (
           <JumpLinksItem key={section} href={`#${section}`}>

diff --git a/frontend/src/components/MarkdownView.scss b/frontend/src/components/MarkdownView.scss
@@ -77,9 +77,7 @@
   code {
     padding: 2px 4px;
     font-size: 85%;
-    color: var(
-      --pf-t--global--text--color--status--danger--default
-    ); // Needs context from RHOAI design; unless this code element is always intended to contain error/danger text, a danger token should not be used
+    color: var(--pf-t--global--color--nonstatus--red--default);
   }
 
   pre {

diff --git a/frontend/src/components/OdhAppCard.tsx b/frontend/src/components/OdhAppCard.tsx
@@ -13,6 +13,7 @@ import {
   DropdownItem,
   MenuToggle,
   DropdownList,
+  Label,
 } from '@patternfly/react-core';
 import { EllipsisVIcon, ExternalLinkAltIcon } from '@patternfly/react-icons';
 import { OdhApplication } from '~/types';
@@ -82,8 +83,14 @@ const OdhAppCard: React.FC<OdhAppCardProps> = ({ odhApp }) => {
   };
 
   const dropdownItems = [
-    <DropdownItem key="docs" to={odhApp.spec.docsLink} target="_blank" rel="noopener noreferrer">
-      View documentation <ExternalLinkAltIcon />
+    <DropdownItem
+      isExternalLink
+      key="docs"
+      to={odhApp.spec.docsLink}
+      target="_blank"
+      rel="noopener noreferrer"
+    >
+      View documentation
     </DropdownItem>,
   ];
 
@@ -210,9 +217,8 @@ const OdhAppCard: React.FC<OdhAppCardProps> = ({ odhApp }) => {
           hasNoOffset: true,
           className: undefined,
         }}
-        style={{ paddingRight: 0 }}
       >
-        <BrandImage src={odhApp.spec.img} alt={odhApp.spec.displayName} data-testid="brand-image" />
+        <BrandImage src={odhApp.spec.img} alt="" data-testid="brand-image" />
       </CardHeader>
       <SupportedAppTitle odhApp={odhApp} />
       <CardBody>
@@ -221,7 +227,9 @@ const OdhAppCard: React.FC<OdhAppCardProps> = ({ odhApp }) => {
         odhApp.spec.support !== ODH_PRODUCT_NAME ? (
           <div className="odh-card__partner-badge-container">
             <span className="odh-card__partner-badge" data-testid="partner-badge">
-              {odhApp.spec.category}
+              <Label className={disabled ? 'pf-m-disabled' : undefined} variant="outline">
+                {odhApp.spec.category}
+              </Label>
             </span>
           </div>
         ) : null}

diff --git a/frontend/src/components/OdhCard.scss b/frontend/src/components/OdhCard.scss
@@ -3,22 +3,18 @@
   // Updating the custom overrides would have involved adding more overrides for font colors
   // Removed the custom odh-card.pf-m-current styling as PF cards with the pf-m-current have styling
   // RHOAI feel free to delete above comments
+  & .odh-brand > img {
+    background-color: var(--pf-t--color--white);
+    padding: var(--pf-t--global--spacer--xs);
+    border-radius: var(--pf-t--global--border--radius--small);
+  }
 
   &__partner-badge-container {
     margin-top: calc(var(--pf-t--global--spacer--sm) * -1);
     margin-bottom: var(--pf-t--global--spacer--sm);
   }
 
   &__partner-badge {
-    // Need product design input as to whether badge on disabled card looks oaky
-    background-color: var(--pf-t--global--background--color--secondary--default);
-    border-color: var(--pf-t--global--border--color--on-secondary);
-    border-style: solid;
-    border-width: 1px;
-    border-radius: var(--pf-t--global--spacer--md);
-    font-size: var(--pf-t--global--font--size--body--sm);
-    font-weight: var(--pf-t--global--font--weight--body--default);
-    margin-bottom: var(--pf-t--global--spacer--sm);
     padding: 0 var(--pf-t--global--spacer--sm);
 
     &.m-hidden {
@@ -39,9 +35,6 @@
       }
     }
     &.odh-m-beta {
-      color: var(--pf-t--global--text--color--nonstatus--on-yellow--default);
-      background-color: var(--pf-t--global--color--nonstatus--yellow--default);
-      border-color: var(--pf-t--global--border--color--nonstatus--yellow--default);
       padding: 0 var(--pf-t--global--spacer--md);
     }
   }

diff --git a/frontend/src/components/OdhDocCard.tsx b/frontend/src/components/OdhDocCard.tsx
@@ -174,7 +174,7 @@ const OdhDocCard: React.FC<OdhDocCardProps> = ({
           className: undefined,
         }}
       >
-        <BrandImage src={odhDoc.spec.img || odhDoc.spec.icon || ''} alt={odhDoc.spec.displayName} />
+        <BrandImage src={odhDoc.spec.img || odhDoc.spec.icon || ''} alt="" />
       </CardHeader>
       <CardTitle>
         <Content>

diff --git a/frontend/src/components/OdhExploreCard.tsx b/frontend/src/components/OdhExploreCard.tsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import classNames from 'classnames';
-import { Card, CardHeader, CardBody, Flex, FlexItem } from '@patternfly/react-core';
+import { Card, CardHeader, CardBody, Flex, FlexItem, Label } from '@patternfly/react-core';
 import { OdhApplication } from '~/types';
 import { makeCardVisible } from '~/utilities/utils';
 import EnableModal from '~/pages/exploreApplication/EnableModal';
@@ -66,11 +66,18 @@ const OdhExploreCard: React.FC<OdhExploreCardProps> = ({
                 )}
                 {!odhApp.spec.comingSoon && odhApp.spec.category && (
                   <FlexItem className={badgeClasses} onClick={disabled ? undefined : onSelect}>
-                    <OdhExploreCardTypeBadge category={odhApp.spec.category} />
+                    <OdhExploreCardTypeBadge
+                      isDisabled={disabled}
+                      category={odhApp.spec.category}
+                    />
                   </FlexItem>
                 )}
                 {odhApp.spec.beta && (
-                  <FlexItem className="odh-card__partner-badge odh-m-beta">Beta</FlexItem>
+                  <FlexItem className="odh-card__partner-badge odh-m-beta">
+                    <Label className={disabled ? 'pf-m-disabled' : undefined} color="yellow">
+                      Beta
+                    </Label>
+                  </FlexItem>
                 )}
               </Flex>
             ),
@@ -87,7 +94,7 @@ const OdhExploreCard: React.FC<OdhExploreCardProps> = ({
           },
         })}
       >
-        <BrandImage src={odhApp.spec.img} alt={odhApp.spec.displayName} data-testid="brand-image" />
+        <BrandImage src={odhApp.spec.img} alt="" data-testid="brand-image" />
       </CardHeader>
       <SupportedAppTitle odhApp={odhApp} showProvider />
       <CardBody data-testid="cardbody">{odhApp.spec.description}</CardBody>

diff --git a/frontend/src/components/OdhExploreCardTypeBadge.tsx b/frontend/src/components/OdhExploreCardTypeBadge.tsx
@@ -1,13 +1,17 @@
 import * as React from 'react';
-import { Tooltip } from '@patternfly/react-core';
+import { Tooltip, Label } from '@patternfly/react-core';
 import { OdhApplicationCategory } from '~/types';
 import { ODH_PRODUCT_NAME } from '~/utilities/const';
 
 type OdhExploreCardTypeBadgeProps = {
   category: OdhApplicationCategory | string;
+  isDisabled?: boolean;
 };
 
-const OdhExploreCardTypeBadge: React.FC<OdhExploreCardTypeBadgeProps> = ({ category }) => {
+const OdhExploreCardTypeBadge: React.FC<OdhExploreCardTypeBadgeProps> = ({
+  category,
+  isDisabled,
+}) => {
   let content;
 
   if (category === OdhApplicationCategory.RedHatManaged) {
@@ -24,7 +28,9 @@ const OdhExploreCardTypeBadge: React.FC<OdhExploreCardTypeBadgeProps> = ({ categ
 
   return (
     <Tooltip content={content}>
-      <span>{category}</span>
+      <Label className={isDisabled ? 'pf-m-disabled' : undefined} tabIndex={0} variant="outline">
+        {category}
+      </Label>
     </Tooltip>
   );
 };

diff --git a/frontend/src/components/searchSelector/SearchSelector.tsx b/frontend/src/components/searchSelector/SearchSelector.tsx
@@ -122,10 +122,7 @@ const SearchSelector: React.FC<SearchSelectorProps> = ({
             </MenuSearchInput>
             {searchHelpText && (
               <HelperText>
-                <HelperTextItem
-                  variant="indeterminate"
-                  data-testid={`${dataTestId}-searchHelpText`}
-                >
+                <HelperTextItem data-testid={`${dataTestId}-searchHelpText`}>
                   {searchHelpText}
                 </HelperTextItem>
               </HelperText>