feat: adds a generic top nav component #2296

ganeshsomasundaram-okta · 2024-07-22T21:24:22Z

Summary

Testing & Screenshots

I have confirmed this change with my designer and the Odyssey Design Team.

ganeshsomasundaram-okta · 2024-07-22T21:26:46Z

packages/odyssey-react-mui/src/icons.generated/Aura.tsx

@@ -0,0 +1,45 @@
+/*!


not sure if we already have these icons somewhere.

The icons should be in Figma. we then run a script to pull them into Odyssey.

If they're not in Figma, then when we run the "pull down new icons" script, it will remove this one.

There are other scripts we run to process the icons too. Then we make a PR and get those up.

ganeshsomasundaram-okta · 2024-07-22T21:27:29Z

packages/odyssey-react-mui/src/icons.generated/index.ts

@@ -26,6 +26,8 @@ export * from "./ArrowUnsorted";
 export * from "./ArrowUp";
 export * from "./ArrowUpperLeft";
 export * from "./ArrowUpperRight";
+export * from "./Aura";
+export * from "./AuraWordmark";


plmk if this exists already or if I should combine these 2 into a single svg/icon.

Don't think either of these exist. Does it make sense to create a component like OktaLogo that can render either one? Maybe it takes a variant prop for logoOnly and logoAndWordMark or something?

packages/odyssey-react-mui/src/labs/TopNav.tsx

KevinGhadyani-Okta · 2024-07-24T18:32:37Z

packages/odyssey-react-mui/src/labs/TopNav.tsx

+  const LogoWordmarkStyles = useMemo(
+    () => ({
+      width: "55px",
+      height: "20px",
+      paddingLeft: odysseyDesignTokens.Spacing2,
+    }),
+    [odysseyDesignTokens],
+  );


We should see if we could get these values lined up with design tokens somehow.

Couldn't find any tokens that might work for these props

@ganeshsomasundaram-okta We should use Spacing9 for the width. And we probably don't need to set a height here

Also curious why it needs padding here. I'd think the containing element would provide any necessary padding

thanks @bryancunningham-okta I'll use the Spacing9, it is slightly smaller than the figma spec but it should be close enough. Also, there is a padding/gap between the aura and the workmark in the design. I don't have a container around the aura/workmark.

Ah, got ya. Can we use margin instead then? Makes it more clear what the intention is. And use the logical css property

marginInlineStart: odysseyDesignTokens.Spacing2

removed the logo for now to unlock this PR. We are waiting from the design team to confirm where the logo would go (between top-nav and side-nav). There are some inconsistencies on the logo behavior/location between enduser & admin apps that design team needs to consolidate.

packages/odyssey-react-mui/src/labs/TopNav.tsx

KevinGhadyani-Okta · 2024-07-24T18:46:25Z

packages/odyssey-react-mui/src/labs/TopNav.tsx

+  /**
+   * Pass in an additional componnet like Button that will displayed after the nav link items
+   */
+  AdditionalNavItemComponent?: ReactElement;


This is really open-ended. I don't understand why it's here either. Is this part of the design where we want clickable nav items to have clickable items inside them?

For accessibility reasons, that's typically a big no-no.

Also this:

Suggested change

/**

* Pass in an additional componnet like Button that will displayed after the nav link items

*/

AdditionalNavItemComponent?: ReactElement;

/**

* Pass in an additional component like `Button` that will displayed after the nav link items.

*/

AdditionalNavItemComponent?: ReactElement;

Yes, there's a use case where the workflows UI has an additional button at the end. I have this prop to cover that use case.

packages/odyssey-react-mui/src/labs/TopNav.tsx

KevinGhadyani-Okta · 2024-07-24T18:51:34Z

packages/odyssey-react-mui/src/labs/TopNav.tsx

+const TopNavListItemContainer = styled("li", {
+  shouldForwardProp: (prop) =>
+    prop !== "odysseyDesignTokens" && prop !== "isDisabled",
+})<{
+  odysseyDesignTokens: DesignTokens;
+  isDisabled?: boolean;
+}>(({ odysseyDesignTokens, isDisabled }) => ({
+  display: "flex",
+  alignItems: "center",
+  cursor: isDisabled ? "default" : "pointer",
+  pointerEvents: isDisabled ? "none" : "auto",
+  "& a": {
+    display: "flex",
+    alignItems: "center",
+    padding: `${odysseyDesignTokens.Spacing2} ${odysseyDesignTokens.Spacing4}`,
+    color: `${odysseyDesignTokens.TypographyColorHeading} !important`,
+  },
+  "& a:hover": {
+    textDecoration: "none",
+    backgroundColor: !isDisabled ? odysseyDesignTokens.HueNeutral50 : "inherit",
+  },
+  "& div[role='button']:hover": {
+    backgroundColor: !isDisabled ? odysseyDesignTokens.HueNeutral50 : "inherit",
+  },
+  "& a:focus-visible": {
+    outlineOffset: 0,
+    borderRadius: 0,
+    outlineWidth: odysseyDesignTokens.FocusOutlineWidthMain,
+    backgroundColor: !isDisabled ? odysseyDesignTokens.HueNeutral50 : "inherit",
+  },
+}));


Since we're always doing !isDisabled, would it make sense to use isEnabled instead? It'd make the code easier to read, but I understand it doesn't follow the way HTML does it.

Something to think about. I prefer readability in these cases since these are regular divs and not form elements.

This kind of follows the same design as the other components like SideNav, etc. If you don't have a strong opinion I'll leave it like this for now to match the other code styles.

packages/odyssey-react-mui/src/labs/TopNav.tsx

KevinGhadyani-Okta · 2024-07-24T18:57:03Z

packages/odyssey-react-mui/src/labs/TopNav.tsx

+            <NavItemContentClickContainer
+              odysseyDesignTokens={odysseyDesignTokens}
+            >
+              <TopNavItemLabelContainer
+                odysseyDesignTokens={odysseyDesignTokens}
+              >
+                {label}
+              </TopNavItemLabelContainer>
+            </NavItemContentClickContainer>
+          ) : !href ? (
+            <NavItemContentClickContainer
+              odysseyDesignTokens={odysseyDesignTokens}
+              role="button"
+              tabIndex={0}
+              onClick={onClick}
+              onKeyDown={topNavItemContentKeyHandler}
+            >
+              <TopNavItemLabelContainer
+                odysseyDesignTokens={odysseyDesignTokens}
+              >
+                {label}
+              </TopNavItemLabelContainer>
+            </NavItemContentClickContainer>
+          ) : (
+            <Link href={href} target={target} onClick={onClick}>
+              <TopNavItemLabelContainer
+                odysseyDesignTokens={odysseyDesignTokens}
+              >
+                {label}
+              </TopNavItemLabelContainer>


I'm wondering if we want to add some role attributes in here. Some of these divs are gonna be hard to select without the correct roles define, but looking through here and not seeing it visually, I can't quite say.

hmm, not sure if any of these are clickable/selectable items. We will have links and buttons in the nav and the li -> div container already has a button role when it is not a link

…ents

…kage

ganeshsomasundaram-okta requested a review from a team as a code owner July 22, 2024 21:24

ganeshsomasundaram-okta commented Jul 22, 2024

View reviewed changes

ganeshsomasundaram-okta commented Jul 23, 2024

View reviewed changes