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

Created Button component #114

Merged

Conversation

ZaidKhan144
Copy link
Contributor

Description

This PR addresses #111 . It adds the Button component as per the latest design.

Testing

  • Ensure the Button component is in flint.ui/src/components/Button/Button.vue
  • Ensure the Button component has required props
  • Ensure styling as per the Figma designs
  • Ensure the proper Hover state and transition effect on hover
  • To test the <Button /> component you may Import it in flint.ui/src/components/Cards/CardInfoRun.vue and use it like <Button btnText="Run" @onClick="showConfirmRunModal()" /> then navigate to http://localhost:8000/flint/dashboard
  • Verify the Button displays and opens the model on click

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there ZaidKhan144 👋
Thank you and congrats 🎉 for opening your first PR on this project.✨
We will review it soon! Till then you can checkout the README.md for more details on it.
Moja global fosters an open and welcoming environment for all our contributors.🌸 Please adhere to our Code Of Conduct.
Feel free to join us on moja global Private Slack by dropping an email here.👩‍💻

@shloka-gupta
Copy link
Member

shloka-gupta commented Nov 19, 2021

@sohamsshah would you like to have a look?

Copy link
Member

@iamrajiv iamrajiv left a comment

Choose a reason for hiding this comment

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

@ZaidKhan144 Please add stories for the Button you have added in the Storybook.

Copy link
Collaborator

@sohamsshah sohamsshah left a comment

Choose a reason for hiding this comment

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

First of all, thanks @ZaidKhan144 for working on this.

Essentially, what we want is the styles to change based on the props. In the present implementation, the size, color etc. props aren't capable of changing css styles. Also, we would prefer having <Button> Click me </Button> over present implementation <Button :btn-text="Click me" />.

If you want more insights upon how to handle dynamic css vars based on vue props, refer this blog. Feel free to ask if blocked anywhere.

@ZaidKhan144
Copy link
Contributor Author

@sohamsshah No worries man, could you check the props now?

Copy link
Collaborator

@sohamsshah sohamsshah left a comment

Choose a reason for hiding this comment

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

Hey @ZaidKhan144 , thanks for working on it. I have suggested changes, after that it is good to be merged!

flint.ui/src/components/Button/Button.vue Outdated Show resolved Hide resolved
flint.ui/src/components/Button/Button.vue Outdated Show resolved Hide resolved
flint.ui/src/components/Button/Button.vue Outdated Show resolved Hide resolved
flint.ui/src/components/Button/Button.vue Outdated Show resolved Hide resolved
@ZaidKhan144
Copy link
Contributor Author

I have added the Button stories in flint.ui/src/stories/Button/Button.stories.js since there is already an existing file with the same name in flint.ui/src/stories/. Let me know if you guys want to have it in any other location.

@ZaidKhan144 ZaidKhan144 requested a review from iamrajiv November 21, 2021 20:26
Copy link
Collaborator

@sohamsshah sohamsshah left a comment

Choose a reason for hiding this comment

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

LGTM! 🥳

@shloka-gupta shloka-gupta merged commit 15e74d0 into moja-global:master Nov 22, 2021
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.

4 participants