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

feat: adds new side navigation component for unified nav design #2269

Merged
merged 13 commits into from
Jul 8, 2024
Merged

feat: adds new side navigation component for unified nav design #2269

merged 13 commits into from
Jul 8, 2024

Conversation

ganeshsomasundaram-okta
Copy link
Contributor

@ganeshsomasundaram-okta ganeshsomasundaram-okta commented Jun 20, 2024 •
edited
Loading

732684

Summary

  1. Figma link if applicable.

image

Testing & Screenshots

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

image

Video (07-02)

SideNav-0702.mov

@ganeshsomasundaram-okta ganeshsomasundaram-okta requested a review from a team as a code owner June 20, 2024 22:55
@ganeshsomasundaram-okta ganeshsomasundaram-okta changed the title feat: adds new side navigation component for unified nav design feat: adds new side navigation component for unified nav design [DRAFT PR] Jun 20, 2024
@KevinGhadyani-Okta
Copy link
Contributor

We'll wanna figure out the padding for this hover state with Alex:
image

KevinGhadyani-Okta
KevinGhadyani-Okta requested changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@KevinGhadyani-Okta KevinGhadyani-Okta left a comment

Choose a reason for hiding this comment

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

I added some comments today but haven't had time to do a full review yet and ran out of time. I'm scheduling time to do this again tomorrow.

packages/odyssey-react-mui/src/labs/NavAccordion.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/NavAccordion.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/NavAccordion.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/NavAccordion.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/NavAccordion.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
packages/odyssey-storybook/src/components/odyssey-labs/SideNav/SideNav.stories.tsx Outdated Show resolved Hide resolved
packages/odyssey-storybook/src/components/odyssey-labs/SideNav/SideNav.stories.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
@ganeshsomasundaram-okta ganeshsomasundaram-okta changed the title feat: adds new side navigation component for unified nav design [DRAFT PR] feat: adds new side navigation component for unified nav design Jun 28, 2024
jordankoschei-okta
jordankoschei-okta reviewed Jul 1, 2024
View reviewed changes
packages/odyssey-react-mui/src/labs/NavAccordion.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/NavAccordion.tsx Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/SideNav.tsx Show resolved Hide resolved
@jordankoschei-okta
Copy link
Contributor

We'll wanna figure out the padding for this hover state with Alex: image

One approach here is to reduce the padding on the outer container, and then apply padding directly on the menu items. Then we can avoid negative margins and (hopefully) just change the background color of the hovered/selected menu item.

evanmoses-okta
evanmoses-okta reviewed Jul 5, 2024
View reviewed changes
packages/odyssey-react-mui/src/labs/SideNav.tsx Outdated
/**
* Whether the item is expanded by default
*/
isDefaultExpanded?: boolean;
Copy link

Choose a reason for hiding this comment

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

So if i understand correctly this prop is only meaningful if isExpanded is undefined, right? Otherwise it's ignored? It should probably have that in the doc for both props.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, these props become optional based on the other one. isExpanded is duplicate prop that is not needed and I've removed that.

Copy link

Choose a reason for hiding this comment

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

Could you please add that info in the docstring for the property here? Thanks.

chrispulsinelli-okta
chrispulsinelli-okta reviewed Jul 5, 2024
View reviewed changes
Copy link
Contributor

@chrispulsinelli-okta chrispulsinelli-okta left a comment

Choose a reason for hiding this comment

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

Changes look great to me. The one thing to fix is the spelling error. Please address the other comments, if necessary.
Let me know when you have the fixes in place and I'll approve. Nice work!

packages/odyssey-storybook/src/components/odyssey-labs/SideNav/SideNav.stories.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/NavAccordion.tsx Outdated Show resolved Hide resolved
packages/odyssey-react-mui/src/labs/NavAccordion.tsx Show resolved Hide resolved
chrispulsinelli-okta
chrispulsinelli-okta previously approved these changes Jul 5, 2024
View reviewed changes
Copy link
Contributor

@chrispulsinelli-okta chrispulsinelli-okta left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Maybe just double check with @evanmoses-okta as well to make sure all his feedback is addressed too? Leaving an approval

🥇

evanmoses-okta
evanmoses-okta previously approved these changes Jul 8, 2024
View reviewed changes
@oktapp-aperture-okta oktapp-aperture-okta bot merged commit e54a470 into main Jul 8, 2024
2 checks passed
@oktapp-aperture-okta oktapp-aperture-okta bot deleted the gs-OKTA-732684-nav branch July 8, 2024 19:55
stephanieyoshimoto-okta pushed a commit that referenced this pull request Jul 15, 2024
@ganeshsomasundaram-okta @stephanieyoshimoto-okta
feat: adds new side navigation component for unified nav design (#2269)
611bc44 
OKTA-737372 feat: adds new side navigation component for unified nav design
add status pill, selected item, expand/collapse side nav options
refactor, review comments, use odyssey tokens
adds play interaction
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jordankoschei-okta jordankoschei-okta jordankoschei-okta left review comments

@bryancunningham-okta bryancunningham-okta bryancunningham-okta approved these changes

@KevinGhadyani-Okta KevinGhadyani-Okta KevinGhadyani-Okta left review comments

@evanmoses-okta evanmoses-okta evanmoses-okta left review comments

@chrispulsinelli-okta chrispulsinelli-okta chrispulsinelli-okta left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ganeshsomasundaram-okta @KevinGhadyani-Okta @jordankoschei-okta @chrispulsinelli-okta @evanmoses-okta @bryancunningham-okta