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.

Sign up for GitHub

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

Jump to bottom

feat: changed proptype from string/IconSubComponentProps to SubIcon #2026

Merged
merged 3 commits into from
Mar 19, 2024
Merged

feat: changed proptype from string/IconSubComponentProps to SubIcon #2026

merged 3 commits into from
Mar 19, 2024

Conversation

MohammedHaris96
Copy link
Contributor

@MohammedHaris96 MohammedHaris96 commented Mar 18, 2024 •
edited
Loading

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

@MohammedHaris96 MohammedHaris96 requested a review from a team as a code owner March 18, 2024 19:57
YossiSaadi
YossiSaadi reviewed Mar 19, 2024
View reviewed changes
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.

Great! can you please change to a relative path?
I'm not sure it would build fine with the current impl

packages/core/src/components/Modal/ModalHeader/ModalHeader.tsx Outdated
@@ -12,6 +12,7 @@ import Text from "../../Text/Text";
import Heading from "../../Heading/Heading";
import Flex from "../../Flex/Flex";
import styles from "./ModalHeader.module.scss";
import { SubIcon } from "src/types";
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure how it behaves
Can you, for the meantime, change it to a relative path? same as other imports in the file? (../../../types)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @YossiSaadi, allow me some time I'll raise another PR.

packages/core/src/components/Tabs/Tab/Tab.tsx Outdated
import VibeComponentProps from "../../../types/VibeComponentProps";
import { IconType } from "../../Icon/IconConstants";
import { ComponentDefaultTestId, getTestId } from "../../../tests/test-ids-utils";
import styles from "./Tab.module.scss";
import { SubIcon } from "src/types";
Copy link
Contributor

Choose a reason for hiding this comment

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

Same
Can you change it to a relative path? same as other imports in the file? (../../../types)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure @YossiSaadi, allow me some time I'll raise another PR.

@YossiSaadi
Copy link
Contributor

no need for another PR, you can commit another commit to this PR :)

@YossiSaadi
Copy link
Contributor

resolves #2025

@MohammedHaris96
Copy link
Contributor Author

@YossiSaadi done :)

@YossiSaadi YossiSaadi changed the title fix: changed proptype from icon to subIcon feat: changed proptype from string/IconSubComponentProps to SubIcon Mar 19, 2024
@YossiSaadi
Copy link
Contributor

hope you don't mind, I've made a little change to the title :)

  • we want it to be declared as a feat and not necessarily a fix, since it introduces an ability and not fixing a bug for the end user
  • change the type change to be a bit more specific

Let's see how the CI goes and then we'll be able to merge.
Great job on your first contribution to Vibe!

@MohammedHaris96
Copy link
Contributor Author

Thank you so much @YossiSaadi for

hope you don't mind, I've made a little change to the title :)

  • we want it to be declared as a feat and not necessarily a fix, since it introduces an ability and not fixing a bug for the end user
  • change the type change to be a bit more specific

Let's see how the CI goes and then we'll be able to merge. Great job on your first contribution to Vibe!

Thank you so much @YossiSaadi for addressing this. :)

@YossiSaadi YossiSaadi merged commit f30ab8a into mondaycom:master Mar 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YossiSaadi YossiSaadi YossiSaadi left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MohammedHaris96 @YossiSaadi