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

moog16 · 2019-03-20T00:08:19Z

reopening from #751
related #697

…cwebv1_try1

abhiomkar · 2019-03-21T15:12:34Z

packages/menu-surface/index.tsx

@@ -253,62 +250,60 @@ class MenuSurface extends React.Component<MenuSurfaceProps, MenuSurfaceState> {
      getWindowScroll: () => {
        return {x: window.pageXOffset, y: window.pageYOffset};
      },
-      setPosition: (position: Position) => {
+      setPosition: (position: Partial<MDCMenuDistance>) => {


Previously, Position had properties with string type. but MDCMenuDistance has properties with number type. You might need to append px when setting style.

Good catch! It was actually working this way before (as a number). I also verified that it is automatically added on to the value when rendered. Do you think I should still add px?

Ah ok. Didn't know it automatically adds 'px' suffix. is that a React thing?

It must be, but I couldn't fin any documentation on this.

abhiomkar · 2019-03-21T15:14:32Z

packages/menu-surface/index.tsx

@@ -217,15 +212,16 @@ class MenuSurface extends React.Component<MenuSurfaceProps, MenuSurfaceState> {
        this.previousFocus.focus();
      },
      isFirstElementFocused: () =>
-        this.firstFocusableElement &&
+        Boolean(this.firstFocusableElement) &&


Does't typescript has sugar syntax for null checking?

this!.firstFocusableElement === document.activeElement

actually I don't even need the Boolean() part. firstFocusableElement and last will be an HTMLElement or null. Which means that in all cases this.firstFocusableElement === document.activeElement will be valid.

abhiomkar · 2019-03-21T15:17:11Z

packages/menu-surface/index.tsx

-        setTransformOrigin: (transformOrigin: string) => this.setState({transformOrigin}),
+    return {
+      addClass: (className: string) => {
+        const classList = new Set(this.state.classList);


if this.state.classList is already of Set type why do we need to instantiate this with Set again?

If we just add to the existing Set, React will not detect a change meaning there won't be a re-render. React does not do a deep comparison.

abhiomkar · 2019-03-21T15:18:51Z

packages/menu-surface/index.tsx

+        classList.delete(className);
+        this.setState({classList});
+      },
+      hasClass: (className: string) => this.classes.split(' ').includes(className),


This should use classList (Set) state for consistency.

So the naming might be causing confusion, but classList is from the foundationClasses (what I want to rename all of them to). this.classes contains all the classes that are on the menu-surface, including mdc-menu-surface, mdc-menu-surface--fixed, and anything the foundation or user add to the component.

abhiomkar · 2019-03-21T15:21:32Z

packages/menu-surface/index.tsx

+      hasClass: (className: string) => this.classes.split(' ').includes(className),
+      hasAnchor: () => !!this.props.anchorElement,
+      notifyOpen: () => {
+        if (this.registerWindowClickListener) {


Why do we need to register window listener here? Seems like this adapter doing more stuff than it should. We're not doing it in vanilla component implementation?

It actually gets registered during the OPENED_EVENT this.listen(strings.OPENED_EVENT, this.registerBodyClickListener_);, which eventually calls the #adapter.notifyOpen method. Since we don't listen to the events, this is the closest we can get to the vanilla component. What are your thoughts?

I feel it is clearer if we encapsulate these event registrations.

notifyOpen: () => { this.handleOpen_(); this.props.onOpen!(); }

So you can put more stuff in handleOpen_ in feature if need to be. Same for notifyClose.

WDYT?

Ya that would clean up the adapter. Sounds good to me

abhiomkar · 2019-03-21T15:34:42Z

packages/menu-surface/index.tsx

+        this.props.onClose!();
+      },
+      isElementInContainer: (el: HTMLElement) => {
+        if (!this.menuSurfaceElement.current) return false;


.contains also includes the root element so you don't need to compare it with root yourself.

The adapter implementation can be shortened to:

this.menuSurfaceElement!.current.contains(el);

good to know!

abhiomkar · 2019-03-21T15:36:35Z

packages/menu-surface/index.tsx

+        return this.menuSurfaceElement.current.contains(el);
+      },
+      isRtl: () => {
+        if (!this.menuSurfaceElement) return false;


I've seen many places that you're checking if this.menuSurfaceElement is null or undefined. Would this every be null or undefined? Can we not use Non-null assertion operator? (this.menuSurfaceElement!.current)

There is a time when this could be undefined/null, which is before componentDidMount, and after componentWillUnmount. So assuming that it is non-null is not accurate.

abhiomkar

LGTM. Responded to your other comments.

abhiomkar · 2019-03-25T17:54:53Z

packages/menu-surface/index.tsx

@@ -253,62 +250,60 @@ class MenuSurface extends React.Component<MenuSurfaceProps, MenuSurfaceState> {
      getWindowScroll: () => {
        return {x: window.pageXOffset, y: window.pageYOffset};
      },
-      setPosition: (position: Position) => {
+      setPosition: (position: Partial<MDCMenuDistance>) => {


Ah ok. Didn't know it automatically adds 'px' suffix. is that a React thing?

abhiomkar · 2019-03-25T19:26:07Z

packages/menu-surface/index.tsx

+      hasClass: (className: string) => this.classes.split(' ').includes(className),
+      hasAnchor: () => !!this.props.anchorElement,
+      notifyOpen: () => {
+        if (this.registerWindowClickListener) {


I feel it is clearer if we encapsulate these event registrations.

notifyOpen: () => { this.handleOpen_(); this.props.onOpen!(); }

So you can put more stuff in handleOpen_ in feature if need to be. Same for notifyClose.

WDYT?

…cwebv1_try1

googlebot · 2019-03-25T22:31:42Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

codecov-io · 2019-03-25T22:50:20Z

Codecov Report

Merging #762 into feat/mdcweb-typescript-update will decrease coverage by 0.05%.
The diff coverage is 80.95%.

@@                        Coverage Diff                        @@
##           feat/mdcweb-typescript-update     #762      +/-   ##
=================================================================
- Coverage                          94.33%   94.27%   -0.06%     
=================================================================
  Files                                 73       73              
  Lines                               2963     2968       +5     
  Branches                             459      457       -2     
=================================================================
+ Hits                                2795     2798       +3     
  Misses                                60       60              
- Partials                             108      110       +2

Impacted Files	Coverage Δ
packages/menu-surface/index.tsx	`83.64% <80.95%> (-0.77%)`	⬇️

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 ea9dfa0...47f5fc2. Read the comment docs.

moog16 · 2019-03-25T23:08:33Z

closing for #774

gugu and others added 12 commits March 19, 2019 16:42

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

23bdb77

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

e14f309

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

9523dc2

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

45926d9

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

007acf6

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

5c762e5

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

aeb44aa

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

b41bb53

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

4e8338a

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

293e39f

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

1583dc3

fix: lint

a70e163

moog16 assigned abhiomkar Mar 20, 2019

googlebot added the cla: yes label Mar 20, 2019

moog16 mentioned this pull request Mar 20, 2019

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

Closed

Matt Goo added 2 commits March 19, 2019 17:24

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

2639599

Merge branch 'feat/mdcweb-typescript-update' into feat/menusurface/md…

43a3a22

…cwebv1_try1

abhiomkar suggested changes Mar 21, 2019

View reviewed changes

fix: pr updates

6ae4216

abhiomkar approved these changes Mar 25, 2019

View reviewed changes

fix: add a handleOpen and handleClose method

32659ab

moog16 force-pushed the feat/mdcweb-typescript-update branch from 2639599 to bc2b36b Compare March 25, 2019 21:17

Merge branch 'feat/mdcweb-typescript-update' into feat/menusurface/md…

b7c3f3a

…cwebv1_try1

googlebot added cla: no and removed cla: yes labels Mar 25, 2019

fix: add react ripple to card

47f5fc2

moog16 mentioned this pull request Mar 25, 2019

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

Merged

moog16 closed this Mar 25, 2019

moog16 deleted the feat/menusurface/mdcwebv1_try1 branch March 25, 2019 23:08

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

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

Uh oh!

Conversation

moog16 commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhiomkar Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhiomkar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Mar 25, 2019

Uh oh!

codecov-io commented Mar 25, 2019

Codecov Report

Uh oh!

moog16 commented Mar 25, 2019

Uh oh!

Uh oh!

moog16 commented Mar 20, 2019 •

edited

Loading

abhiomkar Mar 21, 2019 •

edited

Loading