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

Refactor icon-button to accept SVG or image icons #358

Merged
merged 3 commits into from
Aug 12, 2019
Merged

Conversation

dfreedm
Copy link
Collaborator

@dfreedm dfreedm commented Aug 10, 2019

Refactor <mwc-icon-button> to have slots, allowing SVG and image icons.

Icon must have slot="onIcon"attribute for non-toggling buttons, and an additional Icon with slot="offIcon" for toggling buttons

Example:

<mwc-icon-button>
  <img src="on.png" slot="onIcon">
  <img src="off.png" slot="offIcon">
</mwc-icon-button>

TODO for PR:

  • svg example
  • image example
  • fix tests

Fixes #156

@dfreedm dfreedm requested a review from aomarks August 10, 2019 00:23
Copy link
Contributor

@aomarks aomarks left a comment

Choose a reason for hiding this comment

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

Looks like a nice, elegant solution!

@@ -27,28 +27,36 @@ export class IconButtonBase extends BaseElement {

@query('.mdc-icon-button') protected mdcRoot!: HTMLElement;

@query('slot[name="offIcon"]') protected offIconSlot!: HTMLSlotElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a very annoying bug that means we can't use the HTMLSlotElement type in this position, explained here: https://github.com/material-components/material-components-web-components/blob/c8e187b2f74a62902d418b6b5963052e49e19fa0/packages/formfield/src/mwc-formfield-base.ts#L75

It means that when you compile with emitDecoratorMetadata (which we do internally), then this code breaks in IE/Edge.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

this.on = !this.on;
this.mdcFoundation.handleClick();
}
}

protected calculateShouldToggle() {
this.shouldToggle =
!(this.offIcon === '' && !this.offIconSlot.assignedNodes().length);
Copy link
Contributor

Choose a reason for hiding this comment

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

Mild nitty preference for this.offIcon !== '' || this.offIconSlot.assignedNodes().length > 0 -- but your call.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

this.icon}</i>
<span class="mdc-icon-button__icon">
<slot name="offIcon" @slotchange="${
this.calculateShouldToggle}"><i class="material-icons">${
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe if you add a newline before the <i> then clang won't try and split on the expression

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, that works!

packages/icon-button/src/icon-button-base.ts Outdated Show resolved Hide resolved
- Add tests with SVG, fix test checks for refactoring
- Add svg and image examples in demo
@dfreedm dfreedm marked this pull request as ready for review August 12, 2019 22:20
@dfreedm dfreedm requested a review from e111077 August 12, 2019 22:21
@aomarks
Copy link
Contributor

aomarks commented Aug 12, 2019

Add CHANGELOG entry? (I've been adding them to the top-level one, not sure about the status of the per-package ones).

@dfreedm
Copy link
Collaborator Author

dfreedm commented Aug 12, 2019

Add CHANGELOG entry? (I've been adding them to the top-level one, not sure about the status of the per-package ones).

I added a line to the top-level changelog.

@dfreedm dfreedm merged commit f30dc4d into master Aug 12, 2019
@dfreedm dfreedm deleted the icon-button-slots branch August 12, 2019 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

slot icon-button icons
3 participants