-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Jstefaniuk/hybridize #30
Conversation
jstefaniuk-d2l
commented
May 10, 2018
•
edited
Loading
edited
- Make the unit tests work
- Use the new way of styling for the polymer 2 version of the date picker ( see here for docs: https://cdn.vaadin.com/vaadin-date-picker/2.0.2/demo/theming.html )
…'t a property and so is malformed until the point where it gets set in 'ready'
…light can find it
d2l-date-picker-style-modules.html
Outdated
<link rel="import" href="../polymer/polymer.html"> | ||
<link rel="import" href="../vaadin-date-picker/vaadin-date-picker-styles.html"> | ||
|
||
<dom-module id="vaadin-overlay-styles" theme-for="vaadin-date-picker-overlay"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prefix should be d2l-
?
…ting the internal value rather than the one passed through formatData), also prefix the style modules with d2l-
… d2l-time-picker rederence
🎈 Seems to be working, ready for review |
bower.json
Outdated
"d2l-intl": "^0.4.1", | ||
"iron-a11y-keys": "^1.0.9", | ||
"app-localize-behavior": "~1.0.2", | ||
"app-localize-behavior": "PolymerElements/app-localize-behavior#1 - 2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed as part of the hybridization, but I'd recommend getting this switched over to d2l-localize-behavior
-- it should let you remove some of your resolution code and will ensure dates are formatted and parsed according to a user's preferences within Brightspace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #22.
bower.json
Outdated
"d2l-polymer-behaviors": "^1.0.0" | ||
"d2l-polymer-behaviors": "^1.0.0", | ||
"vaadin-date-picker": "^2.0.0", | ||
"webcomponentsjs": "^v1.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised you need resolutions behind just webcomponentsjs
....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I added that in earlier on, since now it seems like it's no longer used 👍
d2l-date-picker-style-modules.html
Outdated
} | ||
|
||
[part~="overlay-header"] { | ||
background-color: var(--d2l-color-celestine); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're referencing the colors CSS variables, you should import d2l-colors.html
explicitly -- just in case it gets removed from another import later.
d2l-date-picker-style-modules.html
Outdated
|
||
<dom-module id="d2l-vaadin-date-picker-polymer1-styles"> | ||
<template> | ||
<custom-style> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need the custom-style
inside a dom-module template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm no date-picker expert, but the code seems legit and the tests and demo's run swell.
d2l-date-picker.html
Outdated
// on-value-changed got removed for the Polymer2 version of vaadin-date-picker, | ||
// so for Polymer2 we'll set our own event | ||
this._changeListener = this._handleValueChanged.bind(this); | ||
this.addEventListener('value-changed', this._changeListener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this not work in polymer 1?
Date picker slot