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(ModalFooterButtons): support disable buttons and remove secondary #2016

Conversation

omerg-monday
Copy link
Contributor

Basic

  • Used plop (npm run plop) to create a new component.
  • PR has description.
  • New component is functional and uses Hooks.
  • Component is written in TypeScript.

Style

  • Styles are added to NewComponent.modules.scss file inside the NewComponent folder.
  • Component uses CSS Modules.
  • Design is compatible with Monday Design System.
  • Styles are defined using monday-ui-style tokens
  • Displays correctly in all the themes

Storybook

  • Stories were added to /src/components/NewComponent/__stories__/NewComponent.mdx file.
  • Stories include all flows of using the component.
  • Stories implemented using vibe-storybook-components components and tokens.

Tests

Accessibility

@omerg-monday omerg-monday requested a review from a team as a code owner March 13, 2024 09:50
Copy link
Contributor

@YossiSaadi YossiSaadi left a comment

Choose a reason for hiding this comment

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

Seems cool by me 💪🏼

@talkor I was wondering if we should allow passing all props for each of the button
wdyt? should we consult design about it? as it is abusable

Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for contributing! Let's just remove the disableSecondaryButton

@talkor
Copy link
Member

talkor commented Mar 13, 2024

Seems cool by me 💪🏼

@talkor I was wondering if we should allow passing all props for each of the button wdyt? should we consult design about it? as it is abusable

Yes it was the thought at first but designers don't want to give this flexibility for now

Copy link
Member

@talkor talkor left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

});
fireEvent.click(modalFooterComponent.getByText(PRIMARY_BUTTON_TEXT));
expect(onPrimaryButtonClick).not.toBeCalled();
expect(onSecondaryButtonClick).not.toBeCalled();
Copy link
Member

Choose a reason for hiding this comment

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

This is redundant no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a test to validate that the button is indeed disabled

Copy link
Member

Choose a reason for hiding this comment

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

I mean just this line, there's nothing going on with the secondary button in this test, no?

@talkor
Copy link
Member

talkor commented Mar 13, 2024

Closing #2013
Closing #2012

@talkor talkor merged commit 41d2a99 into mondaycom:master Mar 13, 2024
4 checks passed
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.

3 participants