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.

Sign up for GitHub

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

Jump to bottom

Split icon button toggling into its own component #370

Merged
merged 1 commit into from
Aug 15, 2019
Merged

Split icon button toggling into its own component #370

merged 1 commit into from
Aug 15, 2019

Conversation

dfreedm
Copy link
Collaborator

@dfreedm dfreedm commented Aug 15, 2019 •
edited
Loading

  • This makes the non-togging case more lightweight
  • <mwc-icon-button> for non-toggling use cases remains the same
  • For toggling, use <mwc-icon-button-toggle>
    • icon is now onIcon

Fixes #369
Related to #348

@dfreedm dfreedm requested review from aomarks and e111077 August 15, 2019 00:22
aomarks
aomarks suggested changes Aug 15, 2019
View reviewed changes
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.

Nice! Seeing all this code and imports disappear for the non-toggle version seems like a +1 for this refactoring, since I imagine a lot of users won't be using the toggle feature, and now they'll get a lighter build.

demos/icon-button-toggle.html Outdated Show resolved Hide resolved
packages/icon-button-toggle/README.md Outdated Show resolved Hide resolved
packages/icon-button-toggle/README.md Outdated Show resolved Hide resolved
packages/icon-button/src/mwc-icon-button.ts Outdated Show resolved Hide resolved
demos/icon-button-toggle.html Outdated Show resolved Hide resolved
packages/icon-button-toggle/src/icon-button-toggle-base.ts Outdated Show resolved Hide resolved
packages/icon-button-toggle/src/mwc-icon-button-toggle.ts Outdated Show resolved Hide resolved
packages/icon-button/README.md Show resolved Hide resolved
packages/icon-button/README.md Outdated Show resolved Hide resolved
packages/icon-button/src/icon-button-base.ts Outdated Show resolved Hide resolved
@dfreedm
Split icon button toggling into its own component
c580ebe 
- This makes the non-togging case more lightweight
- `<mwc-icon-button>` for non-toggling use cases remains the same
- For toggling, use `<mwc-icon-button-toggle>`
  - `icon` is now `onIcon`

Related to #348
@dfreedm dfreedm force-pushed the icon-button-toggle branch from 4d8c9e9 to c580ebe Compare August 15, 2019 18:35
e111077
e111077 reviewed Aug 15, 2019
View reviewed changes
Copy link
Contributor

@e111077 e111077 left a comment

Choose a reason for hiding this comment

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

I'd be interested in creation benchmarks of boring icon button vs this button and toggle vs the old one with toggle on and both vs the old one as boring and as toggle on

@dfreedm
Copy link
Collaborator Author

dfreedm commented Aug 15, 2019

I'd be interested in creation benchmarks of boring icon button vs this button and toggle vs the old one with toggle on and both vs the old one as boring and as toggle on

Sounds good! I'll pull that into a new issue.

@dfreedm dfreedm mentioned this pull request Aug 15, 2019
Closed
@dfreedm
Copy link
Collaborator Author

dfreedm commented Aug 15, 2019

Created #372 for benchmarking

@dfreedm dfreedm merged commit 2a274b8 into master Aug 15, 2019
@dfreedm dfreedm deleted the icon-button-toggle branch August 15, 2019 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e111077 e111077 e111077 left review comments

@aomarks aomarks aomarks approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split mwc-icon-button toggling variant into a mwc-icon-button-toggle
4 participants
@dfreedm @aomarks @e111077 @googlebot