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.

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

feat(ModalTopActions): top actions component for internal use #2415

Conversation

YossiSaadi
Copy link
Contributor

@YossiSaadi YossiSaadi commented Sep 5, 2024

ModalTopActions 🔒

image

Which shows next to the Header, at the top of the modal

image

  • When X button is triggered, call the onClose that was assigned by consumer to the onClose of the Modal.
  • Renders the extra header action received initially by Modal.
  • Positioned absolute, not actual part of the Header.

Props:

Name Type Description
renderAction (color?: ModalTopActionsColor) => React.ReactElement<typeof MenuButton | typeof IconButton>
color ModalTopActionsColor 🔒(which is "light" | "dark")
closeButtonAriaLabel string
onClose React.MouseEventHandler

Usage example:

<ModalTopActions
  onClose={() => console.log("Hi!")}
  closeButtonAriaLabel="Close"
  renderAction={() => (
    <MenuButton size={MenuButton.sizes.SMALL} ariaLabel="Options">
      <Menu id="menu" size={Menu.sizes.MEDIUM}>
        <MenuItem icon={Sun} iconType={MenuItem.iconType.SVG} title="The sun" />
        <MenuItem icon={Moon} iconType={MenuItem.iconType.SVG} title="The moon" />
        <MenuItem icon={Favorite} iconType={MenuItem.iconType.SVG} title="And the stars" />
      </Menu>
    </MenuButton>
  )}
/>

https://monday.monday.com/boards/3532715121/pulses/7360589183

| ButtonColor.ON_INVERTED_BACKGROUND;

export interface ModalTopActionsProps {
renderAction?: (color?: ModalTopActionsButtonColor) => React.ReactElement<typeof MenuButton | typeof IconButton>;
Copy link
Contributor Author

@YossiSaadi YossiSaadi Sep 5, 2024

Choose a reason for hiding this comment

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

@shlomitc suggests for it to be

Suggested change
renderAction?: (color?: ModalTopActionsButtonColor) => React.ReactElement<typeof MenuButton | typeof IconButton>;
renderAction?: React.ReactElement<typeof MenuButton | typeof IconButton>;

let's discuss it.
The main motivation for making it a function is the following:

// color on `renderHeaderAction` is "light"
<Modal closeButtonTheme="light" renderHeaderAction={(color) => <IconButton ...props color={color} />

vs

// force the user to type it twice - leaves a place for a mistake
<Modal closeButtonTheme="light" renderHeaderAction={<IconButton ...props color={"light"} />

Copy link
Member

Choose a reason for hiding this comment

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

I think we can support both, if you want the color you can use a function and use it in your component, otherwise you can control it yourself with just passing a component

@YossiSaadi YossiSaadi changed the base branch from master to feat/yossi/new-modal-building-blocks-7359960492 September 8, 2024 09:29
@YossiSaadi YossiSaadi marked this pull request as ready for review September 8, 2024 11:02
@YossiSaadi YossiSaadi requested a review from a team as a code owner September 8, 2024 11:02
| ButtonColor.ON_INVERTED_BACKGROUND;

export interface ModalTopActionsProps {
renderAction?: (color?: ModalTopActionsButtonColor) => React.ReactElement<typeof MenuButton | typeof IconButton>;
Copy link
Member

Choose a reason for hiding this comment

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

I think we can support both, if you want the color you can use a function and use it in your component, otherwise you can control it yourself with just passing a component

@YossiSaadi YossiSaadi merged commit 4999a84 into feat/yossi/new-modal-building-blocks-7359960492 Sep 24, 2024
10 checks passed
@YossiSaadi YossiSaadi deleted the feat/yossi/modaltopactions-component-to-render-close-extra-optional-7360589183 branch September 24, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants