fix(menu-surface): Hoist menu-surface via a portal (#500,#628)

[ghost]

Use ReactDOM.createPortal when hoisting the menu surface so that react can handle mounting and unmounting.

[codecov-io]

Codecov Report

Merging #693 into rc0.11.0 will decrease coverage by 0.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##           rc0.11.0     #693      +/-   ##
============================================
- Coverage      94.8%   94.79%   -0.02%     
============================================
  Files            73       73              
  Lines          2928     2922       -6     
  Branches        448      446       -2     
============================================
- Hits           2776     2770       -6     
  Misses           50       50              
  Partials        102      102

Impacted Files	Coverage Δ
packages/menu-surface/index.tsx	84.41% <100%> (-0.59%)	⬇️

---

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 efbd849...319c364. Read the comment docs.

[moog16]

fixes #628
fixes #500

[moog16]

Changes look good overall. Just some small clean up

[ghost]

I've added a fix for the SSR issue but I'm wondering if it would be better to follow the suggestion here facebook/react#13097 (comment) let me know what you think and I can update the PR if needed.

[moog16]

I ran npm start. Seems like menusurface does not open menu anymore. That also explains the failing screenshot tests. Can you look into this?

[ghost]

It was working locally but I must have missed something. I'll look into it.

[ghost]

Not sure what is up with the build and why the button screenshot tests keep timing out but seems to be unrelated to my changes as far as I can tell. I have fixed the issue with the menu-surface being open for the initial render.

[moog16]

@ben-mckernan button screenshot sometimes does that...it is because there isn't enough time for the webpack-dev-server to start up. Just need to restart the build.

[moog16]

Looks good!

[ghost]

Do you need me to squash the commits or will you do it when merging?

[moog16]

Yes please squash. However the tests are failing...it looks like button, which is something you didn't touch. Also I ran them locally and they pass. Can you update the 35s found in package.json to 40s. It looks like this:

"test:screenshots": "docker run -it --rm --cap-add=SYS_ADMIN -e MDC_GCLOUD_SERVICE_ACCOUNT_KEY=\"${MDC_GCLOUD_SERVICE_ACCOUNT_KEY}\" mdcreact/screenshots /bin/sh -c 'git checkout .; git checkout master; git pull; npm i; /home/pptruser/material-components-web-react/test/screenshot/start.sh; sleep 35s; npm run test:image-diff'",

[moog16]

@ben-mckernan can you also add the ^ to react-dom please?

[ghost]

Yes, of course. Good catch!

[ghost]

Changing the sleep didn't help. But from the build log it looks like the --timeout 30000 should be changed in:

"test:image-diff": "MDC_COMMIT_HASH=$(git rev-parse --short HEAD) MDC_BRANCH_NAME=$(git rev-parse --abbrev-ref HEAD) mocha --require ts-node/register --require babel-core/register --ui tdd --timeout 30000 test/screenshot/diff-suite.tsx"

[moog16]

Hmm not sure if it is the timeout, because its only the first test. This leads me to believe that the startup is not complete. Also the tests all complete within 3s - 10s. If you look here you can see the times for each test to complete.

I'm going to restart the test and see if that fixes it.

[moog16]

Try increasing the 40s to 60s...I'd hate to increase the time to that much, but lets just try it.

[moog16]

Can't believe that didn't work...Trying out what you suggested

[moog16]

@ben-mckernan not sure what's going on. try increasing the timeout for pupeteer in test/screenshot/screenshot.tsx. Please just add the timeout option that looks like this: puppeteer/puppeteer#782 (comment)

[moog16]

I'm wondering if this has something to do with adding react-dom package. That's the only thing I can think of that would slow this down compared to the other PRs that are out.

[ghost]

Increasing timeout for puppeteer fixed the tests.

The webpack build for the screenshot tests already includes react-dom in its entry point so just adding react-dom shouldn't affect them, however the addition of createPortal does increase the size of the bundle produced by the screenshot tests. This seem to be enough to push the initial page load over the timeout threshold.

I don't know enough about how puppeteer works but I would speculate that this threshold would have eventually been reached regardless as new features or tests are added that increase the size of the bundle produced by the screenshot build. So something that might be worth testing is minifying the screenshot bundle to see if that affects the initial page load time.

But your point about react-dom did make me realize that it is not marked as external for the standard build and so it was being bundled with menu-surface. I have added a fix for this.

[moog16]

@ben-mckernan oh man! Thanks for putting that in there (externals). That would have been a huge bundle increase...Anyways, what you last wrote made sense and I agree with it. Sorry this took so long, and I hope it doesn't deter you from contributing again. Thanks so much.