SideNav updates #2402

jordankoschei-okta · 2024-10-29T01:18:45Z

Updates to the SideNav, from @bryancunningham-okta and @jordankoschei-okta

…bc-jk-sidenav-updates

# Conflicts: # packages/odyssey-react-mui/src/labs/UiShell/UiShellContent.tsx

# Conflicts: # packages/odyssey-react-mui/src/labs/TopNav/TopNav.tsx

KevinGhadyani-Okta · 2024-11-01T18:12:50Z

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

    // The box shadow should appear above the footer only if the scrollable region has overflow
    ...(isContentScrollable && {
-      boxShadow: odysseyDesignTokens.DepthHigh,
+      boxShadow: "0px -8px 8px -8px rgba(39, 39, 39, 0.08)",


Is there a reason we couldn't use the shadow from Odyssey itself?

We can add clip-path if this was causing bleed-over (like I did on top nav)

Do we need any support for IE11? Will SIW team use this?

KevinGhadyani-Okta · 2024-11-01T18:13:55Z

packages/odyssey-react-mui/src/labs/SideNav/SideNavFooterContent.tsx

+  "& + &": {
+    marginInlineStart: odysseyDesignTokens.Spacing4,
+    paddingInlineStart: odysseyDesignTokens.Spacing4,
+    borderInlineStart: `1px solid ${odysseyDesignTokens.HueNeutral300}`,


I think we have these 1px solid styles in Odyssey design tokens.

KevinGhadyani-Okta · 2024-11-01T18:15:58Z

packages/odyssey-react-mui/src/labs/SideNav/SideNavFooterContent.tsx

+  const memoizedFooterContent = useMemo(() => {
+    return footerItems?.map((item) => (
+      <StyledFooterItemContainer
+        key={item.id}
+        odysseyDesignTokens={odysseyDesignTokens}
+        role="menuitem"
      >
        {item.href ? (
-          <Link key={item.id} href={item.href}>
-            {item.label}
-          </Link>
+          <Link href={item.href}>{item.label}</Link>
        ) : (
-          <Box component="span" key={item.id}>
-            {item.label}
-          </Box>
-        )}
-        {index < footerItems.length - 1 && (
-          <Box
-            key={`${item.id}-separator`}
-            sx={{
-              marginLeft: odysseyDesignTokens.Spacing4,
-              marginRight: odysseyDesignTokens.Spacing4,
-              color: odysseyDesignTokens.HueNeutral300,
-            }}
-          >
-            |
-          </Box>
+          <Box component="span">{item.label}</Box>
        )}
-      </Box>
+      </StyledFooterItemContainer>
    ));


Is there any reason this is memoized and not inline? We don't have any objects requiring memoization. I'd just move it down because we're creating an in-place component for no reason resulting in unnecessary indirection. If we wanna make a separate component, we should move it to another file.

KevinGhadyani-Okta · 2024-11-01T18:17:23Z

packages/odyssey-react-mui/src/labs/SideNav/SideNavHeader.tsx

+  "svg, img": {
+    height: "100%",
+    width: "auto",
+    maxWidth: "100%",
+  },


We only need this if they're passing those to us and we're not rendering them ourselves.

Since we're rendering the images now, we should be able to put the styles there ourselves. I think the issue is we can't pass props to svg elements in Odyssey.

packages/odyssey-react-mui/src/labs/SideNav/SideNavHeader.tsx

KevinGhadyani-Okta · 2024-11-01T18:20:05Z

packages/odyssey-react-mui/src/labs/SideNav/SideNavItemContent.tsx

+  transition: `backgroundColor ${odysseyDesignTokens.TransitionDurationMain}, color ${odysseyDesignTokens.TransitionDurationMain}`,
+
+  ...(isSelected && {
+    color: `${odysseyDesignTokens.TypographyColorAction} !important`,


I hope we could find a way to remove this !important.

color from MuiLink is getting higher precedence somewhere.

KevinGhadyani-Okta · 2024-11-01T18:20:27Z

packages/odyssey-react-mui/src/labs/SideNav/SideNavItemContent.tsx

-  margin: `${odysseyDesignTokens.Spacing1} 0`,
+  backgroundColor: "unset",
+  borderRadius: odysseyDesignTokens.BorderRadiusMain,
+  lineHeight: 1.5,


What's this line-height doing compared to padding? Just wanna make sure I'm on board.

It's increasing the height of the items to 48px. As opposed to just setting a height on them

same as above. I guess we can use the TypographyLineHeightBody

KevinGhadyani-Okta · 2024-11-01T18:20:40Z

packages/odyssey-react-mui/src/labs/SideNav/SideNavItemContent.tsx

-    padding: contextValue.isCompact
-      ? `${odysseyDesignTokens.Spacing0} ${odysseyDesignTokens.Spacing4} ${odysseyDesignTokens.Spacing0} calc(${odysseyDesignTokens.Spacing4} * ${contextValue.depth})`
-      : `${odysseyDesignTokens.Spacing2} ${odysseyDesignTokens.Spacing4} ${odysseyDesignTokens.Spacing2} calc(${odysseyDesignTokens.Spacing4} * ${contextValue.depth})`,
+    color: `${odysseyDesignTokens.TypographyColorHeading} !important`,


Another !important color.

KevinGhadyani-Okta · 2024-11-01T18:21:03Z

packages/odyssey-react-mui/src/labs/SideNav/SideNavItemContent.tsx

-      outlineStyle: odysseyDesignTokens.FocusOutlineStyle,
-      outlineWidth: odysseyDesignTokens.FocusOutlineWidthMain,
+      outline: "none",
+      boxShadow: `inset 0 0 0 3px ${odysseyDesignTokens.PalettePrimaryMain}`,


Do we have this box shadow in Odyssey?

packages/odyssey-react-mui/src/labs/SideNav/SideNavItemLinkContent.tsx

ganeshsomasundaram-okta · 2024-11-01T20:51:44Z

packages/odyssey-react-mui/src/labs/SideNav/NavAccordion.tsx

+  borderRadius: odysseyDesignTokens.BorderRadiusMain,
+  paddingBlock: odysseyDesignTokens.Spacing3,
+  paddingInline: odysseyDesignTokens.Spacing4,
+  lineHeight: 1.5,


we can use TypographyLineHeightBody here

ganeshsomasundaram-okta

lgtm! Added some comments to review.

…rity mutually exclusive

ganeshsomasundaram-okta

🚢

DES-6714 refactor: alphabetize props feat: add the loading state feat: add more a11y roles feat(odyssey-storybook): add SideNavToggleButton feat(odyssey-storybook): refacor and use new toggle button test: add unit tests for SideNav Merge branch 'bc-jk-sidenav-updates' of github.com:okta/odyssey into bc-jk-sidenav-updates fix: add accessible heading role to sidenav refactor: replace t with useTranslation in imports feat(odyssey-storybook): add translations for toggle button feat(odyssey-storybook): update padding and cleanup footer code feat: add badge to SideNav refactor: updating accessibility roles Merge remote-tracking branch 'origin/main' into bc-jk-sidenav-updates fix: fixes Side nav collapse icon wasn't showing up over the app content fix: fix a11y role violations feat: add overflow scroll to TopNav fix: adjust width of Heading skeleton feat(odyssey-storybook): add custom logo props fix: moves banner above UI Shell # Conflicts: # packages/odyssey-react-mui/src/labs/UiShell/UiShellContent.tsx fix: fixes styling of TopNav slots and responsiveness # Conflicts: # packages/odyssey-react-mui/src/labs/TopNav/TopNav.tsx fix: refactors UI Shell scroll logic into useScrollState fix: minor rename of useScrollState generic feat(odyssey-storybook): more style updates to match Figma feat: adds ability to know if UI Shell is rendered on the page fix: fixes PageTemplate padding for UI Shell mode fix: adds missing clip-path to TopNav shadow fix: fixes border not showing up when scrolling side nav in UI Shell fix: fixes topnav shadow not going over Surface fix: removes unused expandedWidth

jordankoschei-okta added 2 commits October 28, 2024 20:12

refactor: alphabetize props

0521de3

feat: add the loading state

eb2e3c8

jordankoschei-okta assigned jordankoschei-okta and bryancunningham-okta Oct 29, 2024

jordankoschei-okta requested a review from a team as a code owner October 29, 2024 01:18

jordankoschei-okta and others added 25 commits October 29, 2024 11:40

feat: add more a11y roles

ca28c63

feat(odyssey-storybook): add SideNavToggleButton

37e241e

feat(odyssey-storybook): refacor and use new toggle button

760055d

test: add unit tests for SideNav

619e815

Merge branch 'bc-jk-sidenav-updates' of github.com:okta/odyssey into …

7adb13e

…bc-jk-sidenav-updates

fix: add accessible heading role to sidenav

673f8eb

refactor: replace t with useTranslation in imports

c5f2d88

feat(odyssey-storybook): add translations for toggle button

3225267

feat(odyssey-storybook): update padding and cleanup footer code

4431e6e

feat: add badge to SideNav

da6b506

refactor: updating accessibility roles

20b3c7f

Merge remote-tracking branch 'origin/main' into bc-jk-sidenav-updates

53dc12f

fix: fixes Side nav collapse icon wasn't showing up over the app content

d65d3dc

fix: fix a11y role violations

72dff57

feat: add overflow scroll to TopNav

5d29133

fix: adjust width of Heading skeleton

b12e91d

feat(odyssey-storybook): add custom logo props

d1004ab

fix: moves banner above UI Shell

3eb6d35

# Conflicts: # packages/odyssey-react-mui/src/labs/UiShell/UiShellContent.tsx

fix: fixes styling of TopNav slots and responsiveness

8c4f60f

# Conflicts: # packages/odyssey-react-mui/src/labs/TopNav/TopNav.tsx

fix: refactors UI Shell scroll logic into useScrollState

64fda9c

fix: minor rename of useScrollState generic

9076cf9

feat(odyssey-storybook): more style updates to match Figma

084f200

feat: adds ability to know if UI Shell is rendered on the page

fd85509

fix: fixes PageTemplate padding for UI Shell mode

4a571bd

fix: adds missing clip-path to TopNav shadow

33c4554

KevinGhadyani-Okta reviewed Nov 1, 2024

View reviewed changes

packages/odyssey-react-mui/src/labs/SideNav/SideNavHeader.tsx Outdated Show resolved Hide resolved

KevinGhadyani-Okta reviewed Nov 1, 2024

View reviewed changes

packages/odyssey-react-mui/src/labs/SideNav/SideNavItemLinkContent.tsx Outdated Show resolved Hide resolved

bryancunningham-okta and others added 6 commits November 1, 2024 14:27

feat(odyssey-react-mui): updated footer padding and logos dont grow

b696d52

fix: fixes clip-path issue causing top nav to cut-off modals in Admin

33acce7

fix: removes unused commented CSS from side nav

58beae7

feat(odyssey-react-mui): use generic in callback for keyboard event

4c709d9

feat(odyssey-react-mui): use grid for sidenav layout

5d56b05

feat(odyssey-react-mui): updated grid transition timing

542239b

ganeshsomasundaram-okta reviewed Nov 1, 2024

View reviewed changes

ganeshsomasundaram-okta previously approved these changes Nov 1, 2024

View reviewed changes

feat(odyssey-react-mui): add id for button to control

5f97f2c

bryancunningham-okta dismissed ganeshsomasundaram-okta’s stale review via 5f97f2c November 1, 2024 21:00

KevinGhadyani-Okta and others added 5 commits November 1, 2024 16:11

fix: fixes type issues

844e3fc

fix: renames SideNavTest.test.ts to SideNav.test.ts

778b3b3

feat(odyssey-react-mui): updated button position. make count and seve…

a011f0a

…rity mutually exclusive

feat(odyssey-react-mui): updated button focus ring. fix storybook tests

5640ebd

refactor: make specific CSS less specific

53023e9

ganeshsomasundaram-okta approved these changes Nov 4, 2024

View reviewed changes

benlister-okta approved these changes Nov 4, 2024

View reviewed changes

oktapp-aperture-okta bot merged commit 2a520c4 into main Nov 4, 2024
1 check passed

oktapp-aperture-okta bot deleted the bc-jk-sidenav-updates branch November 4, 2024 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SideNav updates #2402

SideNav updates #2402

jordankoschei-okta commented Oct 29, 2024

KevinGhadyani-Okta Nov 1, 2024 •

edited

Loading

bryancunningham-okta Nov 1, 2024

KevinGhadyani-Okta Nov 1, 2024

KevinGhadyani-Okta Nov 1, 2024

KevinGhadyani-Okta Nov 1, 2024

KevinGhadyani-Okta Nov 1, 2024

ganeshsomasundaram-okta Nov 1, 2024 •

edited

Loading

KevinGhadyani-Okta Nov 1, 2024

bryancunningham-okta Nov 1, 2024

ganeshsomasundaram-okta Nov 1, 2024

KevinGhadyani-Okta Nov 1, 2024

KevinGhadyani-Okta Nov 1, 2024

ganeshsomasundaram-okta Nov 1, 2024

ganeshsomasundaram-okta left a comment

ganeshsomasundaram-okta left a comment

SideNav updates #2402

SideNav updates #2402

Conversation

jordankoschei-okta commented Oct 29, 2024

KevinGhadyani-Okta Nov 1, 2024 • edited Loading

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

ganeshsomasundaram-okta Nov 1, 2024 • edited Loading

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

ganeshsomasundaram-okta left a comment

Choose a reason for hiding this comment

ganeshsomasundaram-okta left a comment

Choose a reason for hiding this comment

KevinGhadyani-Okta Nov 1, 2024 •

edited

Loading

ganeshsomasundaram-okta Nov 1, 2024 •

edited

Loading