Skip to content

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.

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

@cmckni3 cmckni3 Nov 7, 2023

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 suggestion 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