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

refactor: button component #10165

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sital002
Copy link
Member

@sital002 sital002 commented Jan 22, 2024

Fixes Issue

Changes proposed

Refactor button component

  1. Added cva and tailwind merge currently it is very hard to follow the style due to alot of optional chaining.
  2. Added jsdocs for type safety
  3. currently we can't use ref on the button component so using forward ref so that the button component can be selected.

Check List (Check all the applicable boxes)

  • My code follows the code style of this project.
  • My change requires changes to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • This PR does not contain plagiarized content.
  • The title of my pull request is a short description of the requested changes.

Screenshots

Note to reviewers

@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jan 22, 2024
@sital002
Copy link
Member Author

Currently the all the button have default style if you approve that we should implement these library and use js doc for type safety I will replace all the button props from primary=true to variant=primary
What do you think @eddiejaoude If we are planning to use Typescript in the future then we don't need js doc but if not i think we should use js doc where it makes more sense

@eddiejaoude
Copy link
Member

This looks interesting, I am worried about the file limit with Vercel by adding more dependencies because are approaching the limit already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants