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

add divider to combo box categories #528

Merged

Conversation

SagiL96
Copy link
Contributor

@SagiL96 SagiL96 commented Feb 13, 2022

Basic

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

Style

  • Styles are added to NewComponent.modules.scss file inside of the NewComponent folder.
  • Component uses CSS Modules.
  • Design is compatible with Monday Design System.

Storybook

  • Stories were added to /src/NewComponent/__stories__/NewComponent.stories.js file.
  • Stories include all flows of using the component.

Tests

@hadasfa hadasfa self-requested a review February 13, 2022 15:38
@@ -27,6 +27,7 @@ const Combobox = forwardRef(
disabled,
options,
categories,
categoriesDivider,
Copy link
Contributor

Choose a reason for hiding this comment

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

please change the prop name to "withCategoriesDivider"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

V

@@ -156,9 +156,14 @@ When having a lot of options, you may use headings to categorize them.
workspace: { id: "Workspaces", label: "Workspaces" }
}), []);
return (
<div className="combobox-stories-styles_row">
Copy link
Contributor

Choose a reason for hiding this comment

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

please wrap examples by using storyDescription component and add titles: "Categories with divider" and without "Categories without divider".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

V

@@ -58,6 +58,11 @@ describe("Combobox renders correctly", () => {
expect(tree).toMatchSnapshot();
});

it("with divider", () => {
const tree = renderer.create(<Combobox categoriesDivider />).toJSON();
Copy link
Contributor

Choose a reason for hiding this comment

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

this will render a combobox without any data inside and therefore without categories.
so - if there will be a bug in the categories dividers it will not fail the test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

V

return (
<div role="group" aria-labelledby={`combox-category-${categoryId}`} key={categoryId}>
<ComboboxCategory category={categories[categoryId]} />
<ComboboxCategory category={categories[categoryId]} divider={categoriesDivider && categoryIndex !== 0} />
Copy link
Contributor

Choose a reason for hiding this comment

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

the divider should be a separate unit from the category itself. let's draw it here instead of pass it to the ComboboxCategory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

V

@hadasfa hadasfa self-requested a review February 15, 2022 07:12
@hadasfa hadasfa merged commit 0b63ee8 into mondaycom:master Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants