Classify things #70
Classify things #70
Conversation
There was a problem hiding this comment.
Cool, definitely a lot cleaner -- thanks for doing this!
A couple things to keep in mind:
- This open pull request, so conflicts will happen
- We are on the cusp of moving to Lit Element -- just trying to get everything in place first before sounding the trumpets -- which is why we've mostly held off on doing these conversions so far. The Polymer 3 code will definitely be easier to move to Lit, but just wanted you to not be surprised if in a couple weeks that announcement happens. :)
d2l-navigation-button-close.js
Outdated
| D2L.PolymerBehaviors.FocusableBehavior, | ||
| D2L.PolymerBehaviors.LocalizeBehavior], PolymerElement) { | ||
| static get is() { | ||
| return 'd2l-navigation-button-close'; |
There was a problem hiding this comment.
Nit: I believe the is business is no longer needed (I can't find it in the Polymer 3 docs), so you could just remove these 3 lines and have d2l-element-name down where you register it below.
|
|
||
| $_documentContainer.innerHTML = `<dom-module id="d2l-navigation-button-close"> | ||
| <template strip-whitespace=""> |
There was a problem hiding this comment.
Polymer 3 doesn't appear to have an equivalent. I'm curious what you're seeing in the browser with these changes -- is the whitespace gone?
There was a problem hiding this comment.
According to this Polymer/polymer#4731 it seems like you can just add the attribute to the template.
I didn't specifically notice anything with whitespace, but i wasn't really looking hard either. Feels like an easy enough thing to add
d2l-navigation-band.js
Outdated
| `; | ||
| } | ||
| } | ||
| window.customElements.define(D2LNavigationBand.is, D2LNavigationBand); |
There was a problem hiding this comment.
Nit: window things are in the global namespace so it's not needed.
| } | ||
| static get template() { | ||
| return html` | ||
| ${highlightStyles} |
There was a problem hiding this comment.
I'm a little worried that this is going to result in those styles being copied into every single instance of the button. Lit has solved this using constructable stylesheets... but I'm not sure if there's an elegant Polymer 3 solution. We're on the cusp of moving to Lit anyway, so maybe this isn't a big deal.
There was a problem hiding this comment.
You're right that it will be stamped into each shadow root. But based on this issue Polymer/polymer#4940 (comment) it sounds like browsers do optimize for that.
There was a problem hiding this comment.
Sweet, that makes me feel better. :)
d2l-navigation-immersive.js
Outdated
| import 'd2l-colors/d2l-colors.js'; | ||
| import 'fastdom/fastdom.js'; | ||
| import ResizeObserver from 'resize-observer-polyfill/dist/ResizeObserver.es.js'; | ||
| import 'd2l-typography/d2l-typography-shared-styles.js'; | ||
| import './d2l-navigation.js'; | ||
| import './d2l-navigation-link-back.js'; | ||
| import './d2l-navigation-shared-styles.js'; | ||
| import { Polymer } from '@polymer/polymer/lib/legacy/polymer-fn.js'; | ||
| import { navigationSharedSytle } from './d2l-navigation-shared-styles.js'; |
I need to do a bit of work on the navigation for Consortiums, however I didn't want to work on code that is just the output of polymer modulizer.
So I took a bit of time to update everything to modern polymer 3. Most things were straight forward ports, my biggest changes are in d2l-navigation-immersive.js (attached and detached > connected/disconnected callbacks), and in the way styles are shared (exported const instead of include="").
Please let me know what you think. My hope was that this code would be easier to work with now, and potentially easier to port to lit.