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(icon-button): Remove unused styles and update docs #2957

Merged
merged 4 commits into from
Jun 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions packages/mdc-icon-button/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ npm install @material/icon-button
```scss
@import "@material/icon-button/mdc-icon-button";
```

### JavaScript Instantiation

The icon button will work without JavaScript, but you can enhance it to have a ripple effect by instantiating `MDCRipple` on the root element.
Expand Down Expand Up @@ -81,7 +82,7 @@ The icon button can be used to toggle between an on and off icon. To style an ic
data-toggle-on-content="favorite"
data-toggle-on-label="Remove from favorites"
data-toggle-off-content="favorite_border"
data-toggle-off-label="Add to favorites">favorites_border</button>
data-toggle-off-label="Add to favorites">favorite_border</button>
```

```js
Expand All @@ -101,10 +102,27 @@ Attribute | Description
`data-toggle-<TOGGLE STATE>-content` | The text content to set on the element. Note that if an inner icon is used, the text content will be set on that element instead.
`data-toggle-<TOGGLE STATE>-class` | A CSS class to apply to the icon element. The same rules regarding inner icon elements described for `content` apply here as well.

#### Icon Button Toggle with Font Awesome

The icon button toggle can be used with other font libraries such as Font Awesome that use an inner icon element.

```html
<button id="star-this-item"
class="mdc-icon-button"
aria-label="Unstar this item"
aria-hidden="true"
aria-pressed="true"
data-toggle-on-class="fa-star"
data-toggle-on-label="Unstar this item"
data-toggle-off-class="fa-star-o"
data-toggle-off-label="Star this item"><i class="fa fa-2x fa-star"></i></button>
```

### Icons

The icon button can be used with a standard icon library or an `svg`. The icon button toggle should only be used with
an standard icon library. We recommend you use [Material Icons](https://material.io/tools/icons) from Google Fonts.
The icon button can be used with a standard icon library such as Material Icons or Font Awesome, or with an `svg`.
The icon button toggle should only be used with an standard icon library. We recommend you use
[Material Icons](https://material.io/tools/icons) from Google Fonts.

### Disabled

Expand Down
9 changes: 0 additions & 9 deletions packages/mdc-icon-button/adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,6 @@ class MDCIconButtonToggleAdapter {
/** @param {string} text */
setText(text) {}

/** @return {number} */
getTabIndex() {}

/** @param {number} tabIndex */
setTabIndex(tabIndex) {}

/**
* @param {string} name
* @return {string}
Expand All @@ -76,9 +70,6 @@ class MDCIconButtonToggleAdapter {
*/
setAttr(name, value) {}

/** @param {string} name */
removeAttr(name) {}

/** @param {!IconButtonToggleEvent} evtData */
notifyChange(evtData) {}
}
Expand Down
11 changes: 3 additions & 8 deletions packages/mdc-icon-button/foundation.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,27 +39,23 @@ class MDCIconButtonToggleFoundation extends MDCFoundation {
registerInteractionHandler: (/* type: string, handler: EventListener */) => {},
deregisterInteractionHandler: (/* type: string, handler: EventListener */) => {},
setText: (/* text: string */) => {},
getTabIndex: () => /* number */ 0,
setTabIndex: (/* tabIndex: number */) => {},
getAttr: (/* name: string */) => /* string */ '',
setAttr: (/* name: string, value: string */) => {},
removeAttr: (/* name: string */) => {},
notifyChange: (/* evtData: IconButtonToggleEvent */) => {},
};
}

constructor(adapter) {
super(Object.assign(MDCIconButtonToggleFoundation.defaultAdapter, adapter));

const {ARIA_PRESSED} = MDCIconButtonToggleFoundation.strings;

/** @private {boolean} */
this.on_ = false;
this.on_ = this.adapter_.getAttr(ARIA_PRESSED) === 'true';

/** @private {boolean} */
this.disabled_ = false;

/** @private {number} */
this.savedTabIndex_ = -1;

/** @private {?IconButtonToggleState} */
this.toggleOnData_ = null;

Expand All @@ -72,7 +68,6 @@ class MDCIconButtonToggleFoundation extends MDCFoundation {

init() {
this.refreshToggleData();
this.savedTabIndex_ = this.adapter_.getTabIndex();
this.adapter_.registerInteractionHandler('click', this.clickHandler_);
}

Expand Down
5 changes: 1 addition & 4 deletions packages/mdc-icon-button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,8 @@ class MDCIconButtonToggle extends MDCComponent {
registerInteractionHandler: (type, handler) => this.root_.addEventListener(type, handler),
deregisterInteractionHandler: (type, handler) => this.root_.removeEventListener(type, handler),
setText: (text) => this.iconEl_.textContent = text,
getTabIndex: () => /* number */ this.root_.tabIndex,
setTabIndex: (tabIndex) => this.root_.tabIndex = tabIndex,
getAttr: (name, value) => this.root_.getAttribute(name, value),
getAttr: (name) => this.root_.getAttribute(name),
setAttr: (name, value) => this.root_.setAttribute(name, value),
removeAttr: (name) => this.root_.removeAttribute(name),
notifyChange: (evtData) => this.emit(MDCIconButtonToggleFoundation.strings.CHANGE_EVENT, evtData),
});
}
Expand Down
6 changes: 0 additions & 6 deletions packages/mdc-icon-button/mdc-icon-button.scss
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,4 @@
@include mdc-states;
}

.mdc-icon-button--disabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be replaced with a :disabled selector instead of removed completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The :disabled selector is in the mixins file. This was left over from icon-toggle and was used when the icon-toggle was on an element other than a button (span, div, etc). We dropped support for everything except for button and a tags, so this is no longer needed at all. Like mdc-button you can use :disabled for button elements to disable them. Anchor tags don't support disabled at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet, thanks for explaining!

@include mdc-theme-prop(color, text-disabled-on-light);

pointer-events: none;
}

// postcss-bem-linter: end
15 changes: 12 additions & 3 deletions test/unit/mdc-icon-button/foundation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,24 @@ test('exports cssClasses', () => {
test('defaultAdapter returns a complete adapter implementation', () => {
verifyDefaultAdapter(MDCIconButtonToggleFoundation, [
'addClass', 'removeClass', 'registerInteractionHandler', 'deregisterInteractionHandler',
'setText', 'getTabIndex', 'setTabIndex', 'getAttr', 'setAttr', 'removeAttr', 'notifyChange',
'setText', 'getAttr', 'setAttr', 'notifyChange',
]);
});

const setupTest = () => setupFoundationTest(MDCIconButtonToggleFoundation);

test('#constructor sets on to false', () => {
const {foundation} = setupTest();
assert.isNotOk(foundation.isOn());
const {mockAdapter} = setupTest();
td.when(mockAdapter.getAttr(strings.ARIA_PRESSED)).thenReturn('false');
const foundation = new MDCIconButtonToggleFoundation(mockAdapter);
assert.isFalse(foundation.isOn());
});

test('#constructor sets on to true if the toggle is pressed', () => {
const {mockAdapter} = setupTest();
td.when(mockAdapter.getAttr(strings.ARIA_PRESSED)).thenReturn('true');
const foundation = new MDCIconButtonToggleFoundation(mockAdapter);
assert.isTrue(foundation.isOn());
});

test('#toggle flips on', () => {
Expand Down
18 changes: 0 additions & 18 deletions test/unit/mdc-icon-button/mdc-icon-button-toggle.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,17 +158,6 @@ test('#adapter.setText sets the text content of the inner icon element when used
assert.equal(root.querySelector('#icon').textContent, 'foo');
});

test('#adapter.getTabIndex returns the tabIndex of the element', () => {
const {component} = setupTest({tabIndex: 4});
assert.equal(component.getDefaultFoundation().adapter_.getTabIndex(), 4);
});

test('#adapter.setTabIndex sets the tabIndex of the element', () => {
const {root, component} = setupTest({tabIndex: 4});
component.getDefaultFoundation().adapter_.setTabIndex(2);
assert.equal(root.tabIndex, 2);
});

test('#adapter.getAttr retrieves an attribute from the root element', () => {
const {root, component} = setupTest();
root.setAttribute('aria-label', 'hello');
Expand All @@ -181,13 +170,6 @@ test('#adapter.setAttr sets an attribute on the root element', () => {
assert.equal(root.getAttribute('aria-label'), 'hello');
});

test('#adapter.removeAttr removes an attribute from the root element', () => {
const {root, component} = setupTest();
root.setAttribute('aria-label', 'hello');
component.getDefaultFoundation().adapter_.removeAttr('aria-label');
assert.isNotOk(root.hasAttribute('aria-label'));
});

test(`#adapter.notifyChange broadcasts a ${MDCIconButtonToggleFoundation.strings.CHANGE_EVENT} custom event`, () => {
const {root, component} = setupTest();
const handler = td.func('custom event handler');
Expand Down