feat: ButtonPopover and UserProfilePopover components #2406

benschell-okta · 2024-11-04T19:09:38Z

OKTA-826088

Summary

A few proposed new (Labs) components:

ButtonPopover: uses a <Button> to trigger a MUI <Popover> to appear with some basic padding
UserProfilePopover: Similarly, uses a (MUI) <Button> with a <UserProfile> as the contents to trigger a MUI <Popover> to appear with some basic padding
Also UserProfile stories to storybook (alongside stories for each of the above). Added a new prop to UserProfile to have an icon next to the username (to have the v icon as pictured in Figma)

UserProfile needs additional styling, but that's true regardless of this new addition.

Testing & Screenshots

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

ButtonPopover:

UserProfilePopover:

Practical usage in UIShell-based Admin:

bryancunningham-okta · 2024-11-04T19:41:50Z

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

+    <>
+      <Button
+        {...buttonProps}
+        isDisabled={buttonProps.isDisabled || isOpen}


If this button is disabled when it's open, how can it be closed?

The MUI Popover handles closing (it handles an overlay and clicking anywhere outside of the popover closes it). Kinda like Modal IIRC. Popover has an onClose handler to let you know when it has closed.

I don't really feel like we really gain anything by disabling this button when the menu is open do we? Feels like we may as well just leave it enabled. Was there a certain issue you ran into that this was solving?

Not functionally, no. This was purely around the presentation, specifically this type of view: https://private-user-images.githubusercontent.com/116028555/382894810-99f3a5be-3e71-47b5-abb6-a648c729e2c6.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzA3NTA5NDQsIm5iZiI6MTczMDc1MDY0NCwicGF0aCI6Ii8xMTYwMjg1NTUvMzgyODk0ODEwLTk5ZjNhNWJlLTNlNzEtNDdiNS1hYmI2LWE2NDhjNzI5ZTJjNi5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjQxMTA0JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI0MTEwNFQyMDA0MDRaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT02MGRmM2Q3NjI0ZjU5NDViZjcxZDA1ZWM1ZmQ2ODcwMmE2MzU3MmI1Y2UyMWEyOGJkM2E4YzdjZGMyMWFiNDViJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.BArm2CApDOvnKyjNI4FGHz5ZcOokrGuKQZARaZrTaHQ

The UserProfile piece has no border or anything when not hovered. There is no design in Figma that shows how it should change when interactive (hovering/active), so I just took a guess: I figured we'd want something like the disabled view. So what I did here is what you can see in that screenshot.

bryancunningham-okta · 2024-11-04T19:44:31Z

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

@@ -37,6 +38,7 @@ const UserProfileIconContainer = styled("div", {
 const UserProfileInfoContainer = styled("div")(() => ({
  display: "flex",
  flexDirection: "column",
+  textAlign: "left",


Doesn't really seem like we need any of these styles. Fairly certain we can remove this styled component entirely and render either a plain div or a fragment

The textAlign is definitely needed (the default styles for <button> have text-align: center). As for the rest 🤷 ; the UserProfile component itself needs work more generally, I was just making modifications where necessary to get this Popover to work.

No worries. Just figured I'd flag it. If you need the text-align for a button, I'd probably just pull that button into it's own styled component and apply this there. That way it's a bit more clear what is happening/why this declaration is here.

I think I wasn't very clear in my description.

In UserProfilePopover, I was wrapping this component with a MuiButton (here). In that scenario, a text-align from the button is causing a different layout here in the UserProfile. So this is to reset that / enforce the intended design of this component.

Thanks for the extra context, I seen where you were wrapping this in a button. I was trying to say we should style that^ button with textAlign: "left" directly, rather than applying it here. The reasoning for the text align declaration would be more clear in that scenario, I think

I think my point, though, is that the UserProfile component should always be left aligned regardless of any container it is put in. In that case, it should ensure that it always has the correct alignment no matter what, which is why I'd advocate for having it here at a minimum. Basically, I'm suggesting we insulate this from any consumer doing something silly since this component is exported from Labs by itself as well.

tl;dr: what if someone did <center><UserProfile /></center>? We'd want the UserProfile component to render with proper alignment no matter what.

bryancunningham-okta · 2024-11-04T19:47:12Z

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

+  children: ReactNode | NullElement;
+} & ButtonProps;
+
+const ButtonPopover = (props: ButtonPopoverProps) => {


Can we name this something like... ButtonWithPopover or something similar? Just something that more accurately indicates what this component is/does? The current name leads me to believe that this component is just the popover. But, maybe that's just me. Any thoughts?

I have zero opinions and am happy to name it whatever. This PR Is mainly to start a conversation (and get design blessing)

Got ya, sounds good. Let's go with adaptable ButtonWithPopover for now then, if you don't mind :)

bryancunningham-okta · 2024-11-04T19:48:52Z

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

+
+        disabled={isOpen}
+        onClick={openPopover}
+        variant={isOpen ? "secondary" : "floating"}


Changing variant based on state isn't something we do anywhere else in the system. I'd probably recommend just leaving this in the floating style regardless of it's state

The reason I did this is that the "secondary" variant has a gray background, allowing you to see the outline of the triggering component, while "floating" more closely aligns with the way UserProfile is shown in Figma.

Frankly, in general, this PR is to start a conversation; there's nothing here that's been reviewed by designers.

benschell-okta · 2024-11-04T20:15:57Z

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

+    <>
+      <MuiButton
+        {...muiProps}
+        // aria-controls={ariaControls}


I copied this from <Button> 's impl. I wasn't clear if / how many of these types of things should be set on something using <MuiButton>, so I commented them as a reminder to discuss this. Probably mostly a q for @KevinGhadyani-Okta

DES-6713 feat: smarter column expansion

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

DES-6722 fix: update roadmap data fix: update text fix: update roadmap fix: update data

DES-6729 feat(odyssey-react-mui): smaller nav items and focus ring feat(odyssey-react-mui): increase inline padding to make room for button feat(odyssey-react-mui): updated Surface appearance for white backgrounds feat(odyssey-react-mui): build contrastmode into Surface feat(odyssey-react-mui): updated taggle handle color and timing

OKTA-818689 chore(odyssey-react-mui): add namespace

DES-6753 test: fix broken SideNav tests fix: change button role to button component fix: revert broken change

OKTA-824071 feat: adds dran-n-drop feature to the sidenav feat(odyssey-react-mui): smoother transition fix: sidenav props in ui-shell stories

DES-6761 feat(odyssey-react-mui): add custom pagination props to DataView feat(odyssey-react-mui): fix props import path

benschell-okta · 2024-11-15T15:21:40Z

Closing in favor of the new #2414

KevinGhadyani-Okta · 2024-11-15T20:13:36Z

packages/odyssey-react-mui/src/Buttons/BaseButton.tsx

@@ -225,7 +204,7 @@ const Button = ({
          type={type}
          variant={variant}
        >
-          {label}
+          {children ?? label}


Not sure why we're adding the children prop here.

KevinGhadyani-Okta · 2024-11-15T20:16:56Z

packages/odyssey-react-mui/src/Buttons/BaseMenuButton.tsx

+          <Box
+            sx={{
+              padding: odysseyDesignTokens.Spacing4,
+            }}
+          >


We should make sure these styles are memoized.

@KevinGhadyani-Okta this is the initial pass and has been closed. Please look at #2414 instead.

feat: buttonPopover and userProfilePopover components

0def48e

benschell-okta requested a review from a team as a code owner November 4, 2024 19:09

fix: add back disabling the button when popover is opened

1e3de00

bryancunningham-okta reviewed Nov 4, 2024

View reviewed changes

benschell-okta commented Nov 4, 2024

View reviewed changes

jordankoschei-okta and others added 13 commits November 15, 2024 10:00

Smarter column expansion in DataView (#2404)

14607bb

DES-6713 feat: smarter column expansion

build: bump versions for 1.26.0

569733c

docs: update CHANGELOG for 1.26.0

5f3de03

Updates roadmap page data with policies (#2403)

aa628d4

DES-6722 fix: update roadmap data fix: update text fix: update roadmap fix: update data

Add namespace (#2408)

5712ffa

OKTA-818689 chore(odyssey-react-mui): add namespace

Fix broken SideNav tests (#2411)

adc16a0

DES-6753 test: fix broken SideNav tests fix: change button role to button component fix: revert broken change

feat: adds drag-n-drop feature to the sidenav (#2405)

9072e0b

OKTA-824071 feat: adds dran-n-drop feature to the sidenav feat(odyssey-react-mui): smoother transition fix: sidenav props in ui-shell stories

Add custom pagination props to DataView (#2413)

3bd858c

DES-6761 feat(odyssey-react-mui): add custom pagination props to DataView feat(odyssey-react-mui): fix props import path

build: bump versions for 1.27.0

c229591

docs: update CHANGELOG for 1.27.0

f35ca7c

feat: refactor to be based on MenuButton

41969a3

benschell-okta closed this Nov 15, 2024

KevinGhadyani-Okta reviewed Nov 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ButtonPopover and UserProfilePopover components #2406

feat: ButtonPopover and UserProfilePopover components #2406

benschell-okta commented Nov 4, 2024 •

edited

Loading

bryancunningham-okta Nov 4, 2024

benschell-okta Nov 4, 2024

bryancunningham-okta Nov 4, 2024

benschell-okta Nov 4, 2024

bryancunningham-okta Nov 4, 2024

benschell-okta Nov 4, 2024

bryancunningham-okta Nov 4, 2024

benschell-okta Nov 4, 2024

bryancunningham-okta Nov 4, 2024

benschell-okta Nov 4, 2024

bryancunningham-okta Nov 4, 2024

benschell-okta Nov 4, 2024

bryancunningham-okta Nov 4, 2024

bryancunningham-okta Nov 4, 2024

benschell-okta Nov 4, 2024

benschell-okta Nov 4, 2024

benschell-okta commented Nov 15, 2024

KevinGhadyani-Okta Nov 15, 2024

KevinGhadyani-Okta Nov 15, 2024

benschell-okta Nov 18, 2024

feat: ButtonPopover and UserProfilePopover components #2406

feat: ButtonPopover and UserProfilePopover components #2406

Conversation

benschell-okta commented Nov 4, 2024 • edited Loading

Summary

Testing & Screenshots

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

benschell-okta commented Nov 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benschell-okta commented Nov 4, 2024 •

edited

Loading