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

Feature/hadas/flex #456

Merged
merged 13 commits into from
Jan 18, 2022
Merged

Feature/hadas/flex #456

merged 13 commits into from
Jan 18, 2022

Conversation

hadasfa
Copy link
Contributor

@hadasfa hadasfa commented Jan 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


exports[`Flex renders correctly Horizontal display with children 1`] = `
<div
className=""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for some reason it does not add the class names to the test... what should I do?

Copy link
Contributor

Choose a reason for hiding this comment

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

@hadasfa where did you try the add a test with className

@hadasfa hadasfa requested review from orrgottlieb and removed request for orrgottlieb January 13, 2022 13:39
@@ -74,7 +74,11 @@ addParameters({
"*",
"Accessibility",
"Hooks"
]
],
includeName: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

@hadasfa can we upgrade the version of Storybook (it might solve it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we currently in very advanced alpha version. I can try to update to the most recent alpha version (we are few patches behind), that's ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

we can upgrade but could be for a different PR

src/components/Flex/Flex.jsx Outdated Show resolved Hide resolved
src/components/Flex/Flex.jsx Outdated Show resolved Hide resolved
src/components/Flex/Flex.jsx Outdated Show resolved Hide resolved
src/components/Flex/Flex.jsx Outdated Show resolved Hide resolved
src/components/Flex/Flex.module.scss Outdated Show resolved Hide resolved

exports[`Flex renders correctly Horizontal display with children 1`] = `
<div
className=""
Copy link
Contributor

Choose a reason for hiding this comment

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

@hadasfa where did you try the add a test with className

@@ -74,7 +74,11 @@ addParameters({
"*",
"Accessibility",
"Hooks"
]
],
includeName: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

we can upgrade but could be for a different PR

({ className, id, elementType, direction, wrap, children, justify, align, gap, onClick, style }, ref) => {
const componentRef = useRef(null);
const mergedRef = useMergeRefs({ refs: [ref, componentRef] });
const overrideStyle = useMemo(() => ({ ...style, gap: `${gap}px` }), [style, gap]);
Copy link
Contributor

Choose a reason for hiding this comment

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

@hadasfa what happen if i don't pass any gap value?

const componentRef = useRef(null);
const mergedRef = useMergeRefs({ refs: [ref, componentRef] });
const overrideStyle = useMemo(() => ({ ...style, gap: `${gap}px` }), [style, gap]);
const Element = onClick ? Clickable : elementType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Love it! 😎

@hadasfa hadasfa merged commit 0bbd49a into master Jan 18, 2022
@hadasfa hadasfa deleted the feature/hadas/flex branch January 18, 2022 08:56
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