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

fix: actually disabling the button when it's disabled and using href #1039

Closed
wants to merge 1 commit into from

Conversation

polvallverdu
Copy link

Related to #1038

Since it's gathering the props from AnchorElement | ButtonElement, when using href, it's actually an anchor, but the disabled is still available. It would be really useful to have this feature, since it offers a lot more flexibility for my project, and doesn't mislead when looking at the props.

Copy link

changeset-bot bot commented Jan 9, 2025

⚠️ No Changeset found

Latest commit: 42b6f53

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@polvallverdu
Copy link
Author

Should I add a changeset?

@huntabyte
Copy link
Owner

Hey @polvallverdu, I'm not sure if simply rendering a link without an href when disabled is a good practice.

We should do some more investigating into what the right way to handle this. We obviously want to set the tabindex to a negative number, and likely apply aria-disabled="true".

We can also prevent pointer events on the anchor and apply a data-disabled attribute to align with the other components.

@huntabyte
Copy link
Owner

I just found this guide which seems to be a reasonable approach to it:
https://www.scottohara.me/blog/2021/05/28/disabled-links.html

@polvallverdu
Copy link
Author

Agree, but another issue that arises if the anchor tag is maintained is that the styles for disabled button are applied with disabled:, which only works with button tags. That's the main reasoning of using that. I also believe that the correct solution would be not having href on the button and force to use an anchor tag with the button style applied (like shadcn does for react), but I think it would break some code bases.

Let me know your thoughts on the disabled styles when handling this.

@huntabyte
Copy link
Owner

We could probably just apply data-disabled to both of them and then update the button styles to reflect!

@polvallverdu
Copy link
Author

polvallverdu commented Jan 15, 2025

I've been testing some things (like using the data-disabled attribute), but nothing worked.

The issue here is that if href is a prop, the button converts to an anchor tag, and since disabled is not a prop of the anchor tag, even updating the styles makes the button clickable. That's why I opted to convert it directly to a button when disabled, so we don't have to deal with the hassle of also making it not clickable and not update the styles.

@huntabyte
Copy link
Owner

Hey thanks for taking a stab at this, went ahead and merged #1055 as the fix to this and will be doing a release shortly so you have it in your code! Sorry for the delay, was out on travel this week!

@huntabyte huntabyte closed this Jan 19, 2025
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.

2 participants