mdc-icon-button: documentation inconsistencies and dead code, unused adapter methods

[gjdev]

The mdc-icon-button seems to be an inconsistent copy of the deprecated mdc-icon-toggle. Some code that is not used anymore is left in mdc-icon-button. A big part of the implemented functionality isn't documented anymore.

• The mdc-icon-toggle doesn't change the tabIndex of it's component. This seems right as it's documented to be used on anchor and button tags, that already have the proper tabIndex applied by default. However it still exposes the getTabIndex/setTabIndex adapter methods, that are now useless. setTabIndex is never called. getTabIndex is called, but never really used. Both adapter methods should be removed from the code and the documentation.
• The adapter method removeAttr is never called, and should be removed from code and documentation.
• The toggled variant has styling for a disabled button: mdc-icon-button--disabled. This is not documented. It's also weird. More appropriate would be to apply the disabled styling based on the availability of a :disabled selector. (Especially since the foundation/component never use the mdc-icon-button--disabled class.)
• The 'normal' mdc-icon-button doesn't have styling for a disabled button. (This should also be provided based on a disabled selector).
• Just as the mdc-icon-toggle, the toggled variant of the mdc-icon-button allows a child element to be the actual icon, and receive the icon-font classnames or ligatures (to work around issues with pseudo element conflicts between the ripple and the icon font classes). However, most of the documentation for that feature has been stripped, making it a very confusing read. It does still work, so I think the documentation should be fixed.

Furthermore as a separate thought for this component: I don't really understand why the toggled variant works the way it does, with iconOn/iconOff properties that set an icon-font class or ligature. This needs the earlier workarounds for pseudo elements that are difficult to document/grasp. It also excludes the use of SVG icons. Imho a much better approach would be to have an icon-button-toggle DOM with two embedded icons, one marked as the 'on' icon, one as the 'off' icon. This would work with all types of icon-fonts without workarounds for pseudo elements, and would also work with SVG icons. It's also much easier to document for all those different use cases, as the usage would be exactly the same for any kind of icon.

[williamernest]

@gjdev Thanks for filing this issue. Also, thanks for digging through our code and finding these issues. I have a PR open to address most of these concerns regarding dead code, unused adapter methods, and to fix up our documentation a bit.

We recently introdcued the icon-button component and ended up merging the icon-toggle package into icon-button since they're so similar, but didn't take the time to rethink/rebuild the component. We did make some minor changes to the data attributes to make it more clear, but the way you suggested is probably the way we'll move forward when we get a chance to circle back around to this component.

Thanks!

[kfranqueiro]

For purposes of clarity, I'm going to lift the final paragraph of this bug to its own issue. I believe we've resolved the documentation issues here.

Thanks @gjdev for all of your feedback!