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: add button link component #219

Closed
wants to merge 3 commits into from
Closed

feat: add button link component #219

wants to merge 3 commits into from

Conversation

seedy
Copy link
Contributor

@seedy seedy commented Dec 2, 2021 •
edited
Loading

Description

We have use cases for Buttons which hold href

  • Added a LinkButton component
  • Added a story inside Button folder

Do you think it's a good idea adding this component to Faency?

Accessibility concerns

Links and Buttons should not be mixed together

RFC

RFC proposal issue: #218

Close issues

If agreed upon, this PR would close #107

@seedy seedy self-assigned this Dec 6, 2021
matthieuh
matthieuh approved these changes Dec 10, 2021
View reviewed changes
Copy link
Contributor

@matthieuh matthieuh left a comment

Choose a reason for hiding this comment

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

LGTM

@matthieuh matthieuh self-requested a review December 10, 2021 14:03
matthieuh
matthieuh reviewed Dec 10, 2021
View reviewed changes
Copy link
Contributor

@matthieuh matthieuh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

deleted comment: somehow GH is approving PRs only adding a review comment 🤔

@matthieuh matthieuh self-requested a review December 10, 2021 14:06
matthieuh
matthieuh reviewed Dec 10, 2021
View reviewed changes
Copy link
Contributor

@matthieuh matthieuh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

deleted comment: somehow GH is approving PRs only adding a review comment 🤔

@matthieuh matthieuh self-requested a review December 10, 2021 14:09
matthieuh
matthieuh suggested changes Dec 10, 2021
View reviewed changes
Copy link
Contributor

@matthieuh matthieuh left a comment

Choose a reason for hiding this comment

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

How is it different from using <Button as="a" ... syntax?
In the future, I would suggest having proposals as RFC issues to avoid starting any implementation before we validate them as a team.

@matthieuh matthieuh removed their assignment Dec 10, 2021
@seedy
Copy link
Contributor Author

seedy commented Dec 10, 2021

How is it different from using <Button as="a" ... syntax? In the future, I would suggest having proposals as RFC issues to avoid starting any implementation before we validate them as a team.

@matthieuh

I agree with you. As you can read in this RFC proposal, as the pattern is deprecated by Radix-UI, I knew it was very unlikely that we wouldn't follow it.

Next time, I'll wait for RFC review.

@seedy seedy requested a review from matthieuh December 10, 2021 14:53
@seedy seedy assigned matthieuh and unassigned seedy Dec 10, 2021
@seedy seedy assigned seedy and unassigned matthieuh Jan 19, 2022
@seedy
Copy link
Contributor Author

seedy commented Jan 19, 2022

@seedy According to this RFC #218, it would make more sense to reuse asChild pattern for the Button component.

@seedy seedy marked this pull request as draft January 19, 2022 15:03
@seedy seedy closed this Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulocfjunior paulocfjunior paulocfjunior approved these changes

@matthieuh matthieuh Awaiting requested review from matthieuh

Assignees

@seedy seedy

Labels
bot/no-merge kind/proposal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Button] Can't specify 'as' property directly on Button component
3 participants
@seedy @matthieuh @paulocfjunior