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

Added the disabled state to the disabled attribute on the native html button #472

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Chewieez
Copy link

@Chewieez Chewieez commented Sep 16, 2023

I added the disabled toggle state to the disabled attribute on the native html button. This should help users who use the "onClick" event by not allowing that to trigger when the toggle is set to disabled.

Relates to my initial question and issue here: #353 (comment)

@Chewieez
Copy link
Author

This week I had the realization that if someone wants to setup the toggle where you can click to enable it from a disabled state, using the (click) event, this PR change would break that ability. I know that scenario sounds strange, but I had this thought b/c at work we use toggle and in some cases we do allow the user to un-disable the toggle by clicking on it. (not my choice but there are some decent reasons for the setup).

So now I'm wondering if this change may not be the best idea. I'm open to anyone else's thoughts.

@cmckni3
Copy link
Collaborator

cmckni3 commented Oct 30, 2023

This week I had the realization that if someone wants to setup the toggle where you can click to enable it from a disabled state, using the (click) event, this PR change would break that ability. I know that scenario sounds strange, but I had this thought b/c at work we use toggle and in some cases we do allow the user to un-disable the toggle by clicking on it. (not my choice but there are some decent reasons for the setup).

So now I'm wondering if this change may not be the best idea. I'm open to anyone else's thoughts.

Yeah, I was mulling it over. It is a tricky situation.

@cmckni3
Copy link
Collaborator

cmckni3 commented Nov 4, 2023

This week I had the realization that if someone wants to setup the toggle where you can click to enable it from a disabled state, using the (click) event, this PR change would break that ability. I know that scenario sounds strange, but I had this thought b/c at work we use toggle and in some cases we do allow the user to un-disable the toggle by clicking on it. (not my choice but there are some decent reasons for the setup).

So now I'm wondering if this change may not be the best idea. I'm open to anyone else's thoughts.

Mulled this over a bit.

I think the disabled state coming as an input could be managed externally.

Maybe, could add another option that adds the disabled attribute when the disabled state is set to true.

@@ -2,6 +2,7 @@
type="button"
class="switch"
role="switch"
[disabled]="disabled"
Copy link
Collaborator

@cmckni3 cmckni3 Nov 4, 2023

Choose a reason for hiding this comment

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

Suggested change
[disabled]="disabled"
[attr.disabled]="disabled ? 'disabled' : null"

Copy link
Author

Choose a reason for hiding this comment

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

I'll give this change a try, but I don't think it works with the disabled attribute. I've had issues in the past where you end up with disabled="false" in the DOM, and the element is actually disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh yeah. That's right.

It's due to being an attribute. Luckily, this is a button. It should be disabled="disabled" when actually disabling or not have the the attribute at all.

See updated suggsetion and let me know if that helps.

@Chewieez
Copy link
Author

Chewieez commented Nov 7, 2023

Yeah I agree, offering a separate option like [disableInputEl] would be a good idea. (naming up for comment)

@cmckni3
Copy link
Collaborator

cmckni3 commented Nov 7, 2023

Yeah I agree, offering a separate option like [disableInputEl] would be a good idea. (naming up for comment)

Yeah kind of wondering if this should:

  1. Be functionally disabled using HTML disabled attribute
  2. Appear disabled (probably bad UX/A11y)
  3. (1) & (2)

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