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

Fixed a problem that IconButton/IconToggle cannot be disabled #113

Conversation

hallelujahdrive
Copy link
Contributor

IconButton/IconToggle's setDisabled wasn't really working, so I tried implementing it.
After considering several approaches, I chose the implementation that does not instantiate MDCIconButtonToggle from mdc-icon-button and uses the button element placed as a child node.
Since I have set the style of mdc-icon-button to display:contents;, the wrapper has no visual effect and does not affect the layout.
This change may cause a problem that style is not applied as expected when setting style for IconButton/IconToggle using child combinator or sibling combinator in CSS. Someone may need to modify CSS.

The most ideal approach to the problem is to extend HTMLButtonElement, but even the latest elm didn't support custom elements that extend the built-in element. (elm/virtual-dom#156)
I think there are other implementations besides my code, but some compromise will be required anyway.

@aforemny
Copy link
Owner

Hi @hallelujahdrive, thank you very much for this contribution. I will take a look at it as soon as I find the time!

@aforemny aforemny self-assigned this Jul 16, 2020
@aforemny aforemny added the bug Something isn't working label Jul 16, 2020
@aforemny aforemny removed the bug Something isn't working label Oct 16, 2020
@matsjoyce
Copy link
Contributor

Another possible implementation is #130 (I forgot about this one before implementing it) which only fixes IconButton, not IconToggle. Codewise, I think that are similar, but #130 changes less (may or may not be good).

@aforemny
Copy link
Owner

aforemny commented Mar 5, 2021

Hi @hallelujahdrive, I merged this as 66d0967. Thank you for this contribution and thank you for your patience!

@aforemny aforemny closed this Mar 5, 2021
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.

3 participants