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

Allow Button to accept href attribute #15671

Merged
merged 1 commit into from Jul 24, 2021
Merged

Allow Button to accept href attribute #15671

merged 1 commit into from Jul 24, 2021

Conversation

darleendenno
Copy link
Contributor

@darleendenno darleendenno commented Jul 23, 2021

Issue: N/A

References: #4667

What I did

While I am strongly against the practice of having links appear as buttons (and vice versa), this PR aims to fix a TypeScript error when trying to use <Button isLink />.

How to test

  1. Pull down branch, yarn build --core, yarn start
  2. Navigate to Button story
  3. Last button is a "ButtonLink" that accepts an href prop
  • Is this testable with Jest or Chromatic screenshots? Yep, new story added (see Button stories)
  • Does this need a new example in the kitchen sink apps? Nope
  • Does this need an update to the documentation? Nope

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jul 23, 2021

Nx Cloud Report

CI ran the following commands for commit addac42. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@nx-cloud
Copy link

nx-cloud bot commented Jul 23, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit addac42.
Please make sure you set the \ NX_BRANCH\ environment variable in your CI pipeline .

Check the Getting started section to configure the app.


Sent with 💌 from NxCloud.

@darleendenno darleendenno changed the title fix: allow ButtonLink to accept href attribute Allow Button to accept href attribute Jul 23, 2021
@darleendenno
Copy link
Contributor Author

This is blocking work for the referenced issue -- if we can approve early next week I would be very happy ❤️

Copy link

@smithambera smithambera left a comment

Choose a reason for hiding this comment

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

Code seems good to me, although I fully agree with you about using buttons as links 💯

@MichaelArestad
Copy link
Contributor

This looks good to me. I also have opinions on links disguised as buttons, but generally speaking, there is a time and place for it and we should do it sparingly.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

LGTM!

@shilman shilman merged commit 13a2dd7 into next Jul 24, 2021
@shilman shilman deleted the fix/button-link-props branch July 24, 2021 15:42
@shilman shilman added this to the 6.4 PRs milestone Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants