Skip to content
This repository was archived by the owner on Jan 14, 2025. It is now read-only.

fix(icon-button): upgrade to mdc-web v1.1 #792

Merged
merged 1 commit into from Apr 2, 2019
Merged

fix(icon-button): upgrade to mdc-web v1.1 #792

merged 1 commit into from Apr 2, 2019

Conversation

ghost
Copy link

@ghost ghost commented Apr 2, 2019

related #697

@codecov-io
Copy link

Codecov Report

Merging #792 into feat/mdcweb-typescript-update will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@                      Coverage Diff                       @@
##           feat/mdcweb-typescript-update     #792   +/-   ##
==============================================================
  Coverage                          94.09%   94.09%           
==============================================================
  Files                                 73       73           
  Lines                               2981     2981           
  Branches                             459      459           
==============================================================
  Hits                                2805     2805           
  Misses                                64       64           
  Partials                             112      112
Impacted Files Coverage Δ
packages/icon-button/index.tsx 97.91% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d020dfb...c9d89c9. Read the comment docs.

ghost
ghost commented Apr 2, 2019
View reviewed changes
packages/icon-button/index.tsx
return {
addClass: (className: string) =>
this.setState({classList: this.state.classList.add(className)}),
removeClass: (className: string) => {
const classList = new Set(this.state.classList);
const {classList} = this.state;
Copy link
Author

Choose a reason for hiding this comment

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

This change is done because hasClass would otherwise return a false positive if it were called after this. This led to the isOn property in the event argument for notifyChange always being true.

This pattern seems to be used throughout this library and it is a little worrying as it goes against the first two points that react advises about for using state correctly: https://reactjs.org/docs/state-and-lifecycle.html#using-state-correctly

However, from what I can tell, it needs to work this way in order for hasClass to behave correctly.

Copy link

Choose a reason for hiding this comment

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

Gotcha - ya it I realize that setState is async, and that it can become out of sync with hasClass. It does seem wrong that we are relying on changing state directly without calling setState. That is why we were calling new Set() to create a new instance.

We could possibly make the call to props.handleChange call async, by calling it in the componentDidUpdate. We could use a boolean indicating state.classList is updating (let's call it state.classListIsUpdating. When notifyChange gets trigger, we could set another boolean state.hasChangeOccured. (hopefully you're following). The componentDidUpdate statement would look like:

if (!state.classListIsUpdating && prevState.classListIsUpdating && state.hasChangeOccured) {
  this.props.handleChange(...);
  this.setState({hasChangeOccured: false}); // clean up to setup for the next notifyChange 
}

Just a thought as a fix for this. It definitely doesn't belong in this PR, but maybe we can open another issue to fix this.

Copy link

Choose a reason for hiding this comment

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

And we should actually be consistent in the removeClass and addClass methods...
Created an issue #793

Copy link
Author

Choose a reason for hiding this comment

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

@moog16 thanks for the feedback and creating a new issue for this.

moog16
moog16 reviewed Apr 2, 2019
View reviewed changes
test/unit/icon-button/index.test.tsx
});

test('#adapter.addClass adds a class to state.classList', () => {
const wrapper = shallow<IconButton<HTMLButtonElement>>(<IconButton />);
wrapper.instance().adapter.addClass('test-class');
wrapper.state().classList.has('test-class');
assert.isTrue(wrapper.state().classList.has('test-class'));
Copy link

Choose a reason for hiding this comment

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

wow - these tests were basically useless...that's for fixing these.

@moog16 moog16 merged commit 8064955 into material-components:feat/mdcweb-typescript-update Apr 2, 2019
@ghost ghost deleted the fix/icon-button/mdc-v1 branch April 3, 2019 06:08
moog16 pushed a commit that referenced this pull request Apr 30, 2019
fix(icon-button): upgrade to mdc-web v1.1 (#792)
e5b953f
moog16 pushed a commit that referenced this pull request Apr 30, 2019
Squashed commit of the following:
ffcf3d6 
commit 14b22bc
Author: Matt Goo <[email protected]>
Date:   Tue Apr 30 12:06:32 2019 -0700

    feat(select): enhanced select (#823)

commit b400013
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 14:49:32 2019 -0700

    chore: reduce unused deps in package.json (#819)

commit beedb23
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 13:43:20 2019 -0700

    feat(select): add icon (#825)

commit e22ac2a
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 13:35:45 2019 -0700

    feat(select): add helper text (#824)

commit abaa146
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 13:35:16 2019 -0700

    feat(select): add option component (#826)

commit 7adc12a
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 13:11:16 2019 -0700

    fix(drawer): add missing foundation import in drawer (#821)

commit 7e0f877
Author: Matt Goo <[email protected]>
Date:   Fri Apr 26 14:33:40 2019 -0700

    feat(menu): add component (#786)

commit 509e93e
Author: Matt Goo <[email protected]>
Date:   Fri Apr 26 09:46:26 2019 -0700

    chore(infrastructure): add esmoduleinterop to tsconfig  (#818)

commit 4dbc8b8
Author: Matt Goo <[email protected]>
Date:   Tue Apr 16 15:56:25 2019 -0700

    feat(notched-outline): update to v1.1.1 (#806)

commit ea15a2a
Author: 태재영 <[email protected]>
Date:   Sat Apr 13 06:50:37 2019 +0900

     fix(chips): Fix incorrect ripple effect (#804)

commit 7b413df
Author: Matt Goo <[email protected]>
Date:   Mon Apr 8 13:27:36 2019 -0700

    feat(button): update mdc web deps to v1.1.0 (#791)

commit 034158c
Author: Matt Goo <[email protected]>
Date:   Tue Apr 2 17:36:46 2019 -0700

    fix: remove .only (#794)

commit 74379af
Author: Matt Goo <[email protected]>
Date:   Tue Apr 2 16:24:45 2019 -0700

    fix(list): maintain classes with state.listItemClassNames (#776)

    BREAKING CHANGE: Removes props.tabbableOnListItemFocus from all the auxiliary components, as it seemed confusing to have this and tabIndex dictate what tabIndex would ultimately be.

commit e5b953f
Author: Ben McKernan <[email protected]>
Date:   Tue Apr 2 20:18:03 2019 +0200

    fix(icon-button): upgrade to mdc-web v1.1 (#792)

commit 9c7bfa7
Author: Andrii Kostenko <[email protected]>
Date:   Mon Apr 1 23:08:27 2019 +0300

    fix(card): upgrade to mdc-web v1.1 (#788)

commit bb9e7e4
Author: Andrii Kostenko <[email protected]>
Date:   Mon Apr 1 20:44:28 2019 +0300

    fix(top-app-bar): mdc-web v1 upgrade (#780)

commit 37c7269
Author: Andrii Kostenko <[email protected]>
Date:   Mon Apr 1 20:00:39 2019 +0300

    fix(linear-progress): upgrade mdc-web to v1 (#787)

commit b6403e6
Author: Andrii Kostenko <[email protected]>
Date:   Mon Apr 1 19:38:31 2019 +0300

    fix(fab): upgrade to mdc-web v1.1 (#790)

commit bcda111
Author: Andrii Kostenko <[email protected]>
Date:   Fri Mar 29 21:11:19 2019 +0300

    fix(dialog): mdc-web v1 upgrade (#779)

commit 8ff8695
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 28 22:32:55 2019 +0300

    fix(typography): upgrade to mdc-web v1 (#778)

commit 4d6bbc0
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 28 22:15:22 2019 +0300

    feat(radio): upgrade to typescript v1 (#777)

commit 4f523e3
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 28 22:13:29 2019 +0300

    fix(checkbox): upgrade mdc-web to v1 (#769)

commit 1a04f3d
Author: Matt Goo <[email protected]>
Date:   Wed Mar 27 12:39:00 2019 -0700

    feat(menu-surface): upgrade to mdc web v1 (#774)

commit 44cca84
Author: Andrii Kostenko <[email protected]>
Date:   Wed Mar 27 01:14:18 2019 +0200

    feat(switch): upgrade to mdc-web v1 (#757)

commit 580c850
Author: Andrii Kostenko <[email protected]>
Date:   Wed Mar 27 01:13:37 2019 +0200

    fix(tab-bar): upgrade mdc-web to v1 (#770)

commit 45fef89
Author: Matt Goo <[email protected]>
Date:   Mon Mar 25 15:27:29 2019 -0700

    fix(card): add react-ripple to package json (#773)

commit 4e99b4c
Author: Matt Goo <[email protected]>
Date:   Tue Mar 19 14:10:37 2019 -0700

    feat(list): update to MDC Web 1.0.0 (#740)

commit 38e9886
Author: Matt Goo <[email protected]>
Date:   Tue Mar 19 12:36:06 2019 -0700

    feat(floating-label): update mdc web to v1.0.0 (#741)

commit dd95b60
Author: Andrii Kostenko <[email protected]>
Date:   Mon Mar 18 23:38:34 2019 +0200

    feat(snackbar): MDC Web v1.0.0 (#755)

commit 9b86474
Author: Andrii Kostenko <[email protected]>
Date:   Mon Mar 18 23:27:39 2019 +0200

    fix(tab): mdc-web v1.0.0 upgrade (#748)

commit a8c64b5
Author: Lucas Cordeiro <[email protected]>
Date:   Mon Mar 18 17:46:29 2019 -0300

    feat(drawer): add innerRef prop (#749)

commit 5f55983
Author: Andrii Kostenko <[email protected]>
Date:   Fri Mar 15 19:05:44 2019 +0200

    fix(chips): upgrade mdc-web to v1.0.0 (#750)

commit 09cb8a5
Author: Matt Goo <[email protected]>
Date:   Fri Mar 15 10:05:03 2019 -0700

    fix: [email protected] --> [email protected] & update imports (#709)

commit ae9b421
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 14 20:53:28 2019 +0300

    fix(tab-scroller): upgrade mdc-web to 1.0.0 (#743)

commit 1b29dd9
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 14 20:32:18 2019 +0300

    fix(tab-indicator): upgrade mdc-web to 1.0.0 (#742)

commit f966714
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 14 19:45:11 2019 +0300

    feat(ripple) mdc-web typescript conversion (#711)

commit 0b674f5
Author: Andrii Kostenko <[email protected]>
Date:   Fri Mar 8 23:54:24 2019 +0300

    feat(line-ripple): mdc-web typescript support (#716)
moog16 pushed a commit that referenced this pull request May 1, 2019
feat: update mdcweb v1.x.x (#830)
faf3b09 
* Squashed commit of the following:

commit 14b22bc
Author: Matt Goo <[email protected]>
Date:   Tue Apr 30 12:06:32 2019 -0700

    feat(select): enhanced select (#823)

commit b400013
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 14:49:32 2019 -0700

    chore: reduce unused deps in package.json (#819)

commit beedb23
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 13:43:20 2019 -0700

    feat(select): add icon (#825)

commit e22ac2a
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 13:35:45 2019 -0700

    feat(select): add helper text (#824)

commit abaa146
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 13:35:16 2019 -0700

    feat(select): add option component (#826)

commit 7adc12a
Author: Matt Goo <[email protected]>
Date:   Mon Apr 29 13:11:16 2019 -0700

    fix(drawer): add missing foundation import in drawer (#821)

commit 7e0f877
Author: Matt Goo <[email protected]>
Date:   Fri Apr 26 14:33:40 2019 -0700

    feat(menu): add component (#786)

commit 509e93e
Author: Matt Goo <[email protected]>
Date:   Fri Apr 26 09:46:26 2019 -0700

    chore(infrastructure): add esmoduleinterop to tsconfig  (#818)

commit 4dbc8b8
Author: Matt Goo <[email protected]>
Date:   Tue Apr 16 15:56:25 2019 -0700

    feat(notched-outline): update to v1.1.1 (#806)

commit ea15a2a
Author: 태재영 <[email protected]>
Date:   Sat Apr 13 06:50:37 2019 +0900

     fix(chips): Fix incorrect ripple effect (#804)

commit 7b413df
Author: Matt Goo <[email protected]>
Date:   Mon Apr 8 13:27:36 2019 -0700

    feat(button): update mdc web deps to v1.1.0 (#791)

commit 034158c
Author: Matt Goo <[email protected]>
Date:   Tue Apr 2 17:36:46 2019 -0700

    fix: remove .only (#794)

commit 74379af
Author: Matt Goo <[email protected]>
Date:   Tue Apr 2 16:24:45 2019 -0700

    fix(list): maintain classes with state.listItemClassNames (#776)

    BREAKING CHANGE: Removes props.tabbableOnListItemFocus from all the auxiliary components, as it seemed confusing to have this and tabIndex dictate what tabIndex would ultimately be.

commit e5b953f
Author: Ben McKernan <[email protected]>
Date:   Tue Apr 2 20:18:03 2019 +0200

    fix(icon-button): upgrade to mdc-web v1.1 (#792)

commit 9c7bfa7
Author: Andrii Kostenko <[email protected]>
Date:   Mon Apr 1 23:08:27 2019 +0300

    fix(card): upgrade to mdc-web v1.1 (#788)

commit bb9e7e4
Author: Andrii Kostenko <[email protected]>
Date:   Mon Apr 1 20:44:28 2019 +0300

    fix(top-app-bar): mdc-web v1 upgrade (#780)

commit 37c7269
Author: Andrii Kostenko <[email protected]>
Date:   Mon Apr 1 20:00:39 2019 +0300

    fix(linear-progress): upgrade mdc-web to v1 (#787)

commit b6403e6
Author: Andrii Kostenko <[email protected]>
Date:   Mon Apr 1 19:38:31 2019 +0300

    fix(fab): upgrade to mdc-web v1.1 (#790)

commit bcda111
Author: Andrii Kostenko <[email protected]>
Date:   Fri Mar 29 21:11:19 2019 +0300

    fix(dialog): mdc-web v1 upgrade (#779)

commit 8ff8695
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 28 22:32:55 2019 +0300

    fix(typography): upgrade to mdc-web v1 (#778)

commit 4d6bbc0
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 28 22:15:22 2019 +0300

    feat(radio): upgrade to typescript v1 (#777)

commit 4f523e3
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 28 22:13:29 2019 +0300

    fix(checkbox): upgrade mdc-web to v1 (#769)

commit 1a04f3d
Author: Matt Goo <[email protected]>
Date:   Wed Mar 27 12:39:00 2019 -0700

    feat(menu-surface): upgrade to mdc web v1 (#774)

commit 44cca84
Author: Andrii Kostenko <[email protected]>
Date:   Wed Mar 27 01:14:18 2019 +0200

    feat(switch): upgrade to mdc-web v1 (#757)

commit 580c850
Author: Andrii Kostenko <[email protected]>
Date:   Wed Mar 27 01:13:37 2019 +0200

    fix(tab-bar): upgrade mdc-web to v1 (#770)

commit 45fef89
Author: Matt Goo <[email protected]>
Date:   Mon Mar 25 15:27:29 2019 -0700

    fix(card): add react-ripple to package json (#773)

commit 4e99b4c
Author: Matt Goo <[email protected]>
Date:   Tue Mar 19 14:10:37 2019 -0700

    feat(list): update to MDC Web 1.0.0 (#740)

commit 38e9886
Author: Matt Goo <[email protected]>
Date:   Tue Mar 19 12:36:06 2019 -0700

    feat(floating-label): update mdc web to v1.0.0 (#741)

commit dd95b60
Author: Andrii Kostenko <[email protected]>
Date:   Mon Mar 18 23:38:34 2019 +0200

    feat(snackbar): MDC Web v1.0.0 (#755)

commit 9b86474
Author: Andrii Kostenko <[email protected]>
Date:   Mon Mar 18 23:27:39 2019 +0200

    fix(tab): mdc-web v1.0.0 upgrade (#748)

commit a8c64b5
Author: Lucas Cordeiro <[email protected]>
Date:   Mon Mar 18 17:46:29 2019 -0300

    feat(drawer): add innerRef prop (#749)

commit 5f55983
Author: Andrii Kostenko <[email protected]>
Date:   Fri Mar 15 19:05:44 2019 +0200

    fix(chips): upgrade mdc-web to v1.0.0 (#750)

commit 09cb8a5
Author: Matt Goo <[email protected]>
Date:   Fri Mar 15 10:05:03 2019 -0700

    fix: [email protected] --> [email protected] & update imports (#709)

commit ae9b421
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 14 20:53:28 2019 +0300

    fix(tab-scroller): upgrade mdc-web to 1.0.0 (#743)

commit 1b29dd9
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 14 20:32:18 2019 +0300

    fix(tab-indicator): upgrade mdc-web to 1.0.0 (#742)

commit f966714
Author: Andrii Kostenko <[email protected]>
Date:   Thu Mar 14 19:45:11 2019 +0300

    feat(ripple) mdc-web typescript conversion (#711)

commit 0b674f5
Author: Andrii Kostenko <[email protected]>
Date:   Fri Mar 8 23:54:24 2019 +0300

    feat(line-ripple): mdc-web typescript support (#716)

* feat(text-field): update to v1.1.1 (#812)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@codecov-io @moog16 @googlebot