Spike to convert the window top bar to the material framework; part of #1631

[cbeer]

https://deploy-preview-1635--mirador-dev.netlify.com/__tests__/integration/mirador/

🤷‍♂️

Part of #1631 #1633

[sdellis]

This bundle is pretty big. The Minimizing Bundle Size doc recommends two options for reducing bundle size as a temporary fix to tree shaking. Unfortunately, the ability to tree shake material-ui is still being worked out.

However, it looks like this branch follows the Option 1 recommendation, so I'm wondering why material-ui is making the bundle so heavy?

What does the withStyles module do? I wonder if this destructured import requires babel-plugin-import to help reduce the size?

[codecov-io]

Codecov Report

Merging #1635 into master will increase coverage by 0.58%.
The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #1635      +/-   ##
==========================================
+ Coverage   86.29%   86.88%   +0.58%     
==========================================
  Files          33       37       +4     
  Lines         321      343      +22     
==========================================
+ Hits          277      298      +21     
- Misses         44       45       +1

Impacted Files	Coverage Δ
src/components/App.js	60% <ø> (ø)	⬆️
src/components/Window.js	100% <ø> (ø)	⬆️
src/components/WorkspaceControlPanelButtons.js	100% <100%> (ø)
src/components/WindowSideBar.js	100% <100%> (ø)
src/components/WindowTopBar.js	91.66% <100%> (+0.75%)	⬆️
src/components/WindowSideBarButtons.js	100% <100%> (ø)
src/components/WorkspaceControlPanel.js	94.44% <100%> (+2.13%)	⬆️
src/components/ManifestListItem.js	90.9% <100%> (+0.9%)	⬆️
src/components/CompanionWindow.js	66.66% <66.66%> (ø)
... and 4 more

---

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 9f886c7...328cfd0. Read the comment docs.

[camillevilla]

@sdellis thanks for looking into the bundling issues. It would be interesting to put heads together to talk about Material UI’s viability. withStyles returns an HOC with a classes property.

babel-plugin-import sounds useful. Should we add a ticket to implement it outside of this branch?

[sdellis]

I'm seeing the ThumbnailNavigation in here, but I imagine it was not part of this PR. At any rate, the thumbnails need to be keyboard accessible. Changing it to something like this will work:

      <li
        role="presentation"
        className={ns(['thumbnail-nav-canvas', `thumbnail-nav-canvas-${canvas.index}`, this.currentCanvasClass(canvas.index)])}
      >
        <button
          type="button"
          key={canvas.index}
          onClick={() => setCanvas(window.id, canvas.index)}
          onKeyPress={() => setCanvas(window.id, canvas.index)}
        >
          {canvas.index}
        </button>
      </li>

However, the question is whether focus should go to the main content after a thumbnail is clicked (I think so). I'm concerned that a user navigating thumbs of a long book could want to get out of the thumbnails easily at any point, or they may want to focus on the content, but return to the thumbnail list with the thumbnail focus at the current index.

[sdellis]

@camillevilla yes, let's tackle the bundle size in a separate ticket/branch. This PR looks good to me and none of my comments should necessarily stop it from merge I don't think.

[mejackreed]

Do we know if babel-plugin-import will reduce the bundle size yet? I'm just asking the question here as a I'd like to move this forward today, but it might be useful to know if there is a way forward/backward on taking on a large dependency required in our bundle?

[cbeer]

@sdellis you're right, ThumbnailNavigation was added by #1685; I think updating it should be outside the scope of this PR (and, presumably, ought to be completely redone to fit within the window components offered here...)

[sdellis]

@mejackreed we are somehow including the entirety of Material UI in this bundle, and it seems like there are ways to prevent this. We should be able to figure out the treeshaking or destructuring piece in another ticket/branch, so I see that as the way forward. I think it will be more difficult to back out of any such framework, but Material UI in documentation and theory seems to do the things we want. I'm approving this PR for merge and creating a separate ticket for treeshaking now.

[sdellis]

👍 to merge. Unfortunately, theres's a merge conflict with a test that fails because of some configuration in master that is expected outside of the test. Perhaps that configuration should happen within the test, but it's not relevant to this PR other than it's blocking a merge.

[cbeer]

I'll rebase it.

[jkeck]

[mejackreed]

[eps1lon]

Hey, material-ui contributor here. Just stumbled over this since this was referenced from one of my PRs and I wanted to investigate what issues you encountered with tree shaking.

This bundle is pretty big

@sdellis
Do you have some diagnostics about the bundle? Something like a tree map for the bundled files before and after this PR?

You don't actually have to use withStyles if you use material-ui components. It's just the internal styling solution. Since you're already using css you could simply use classnames from your stylesheets and use those in the material-ui components.

[sdellis]

@eps1lon Our treemap only shows the mirador library because we are specifying umd for LibraryTarget in our webpack. My understanding is that we need an es6 build to be able to treeshake. I noticed that umd-module is also an option for LibraryTarget. It wraps es6 in umd. Not sure if that will work, but can test it out.

[mejackreed]

I'm seeing this in our current build:

https://gist.github.com/mejackreed/ae13d154ded8a836891acff3f776cc24#file-stats-json

cc @eps1lon

[sdellis]

Thanks @mejackreed ... It's nice that webpack-bundle-analyzer can do this, and it demonstrates that all of Material-UI is being bundled, even though we are only using a handful of components and importing them as described in the Minimizing Bundle Size Docs.

I switched to importing from the /es directory and I was able to shave ~26KB off "Stat Size", but "Parsed Size" and "Gzipped size" actually increased. And, I'm not convinced it's fully functional since I'm seeing unused modules (such as M-UI's Modal.js) in that map. FWIW, I was not able to pull any icons from the /es library, so perhaps this single import is complicating things? import AddIcon from '@material-ui/icons/Add';

Any thoughts or advice on where to go from here @eps1lon ?