Skip to content
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

2.0-preview branch #64

Merged
merged 17 commits into from
May 12, 2017
Merged

2.0-preview branch #64

merged 17 commits into from
May 12, 2017

Conversation

bicknellr
Copy link
Contributor

No description provided.

Elliott Marquez and others added 3 commits December 7, 2016 17:48
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@bicknellr
Copy link
Contributor Author

I'm deleting a bunch of @rjk40's comments:
screen shot 2017-03-01 at 15 03 16
screen shot 2017-03-01 at 15 03 35

demo/index.html Outdated
}

.set:nth-of-type(4n) {
color: var( --google-blue-500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: stray space before the --

document.querySelector('[is=dom-bind]').getIconNames = function(iconset) {
return iconset.getIconNames();
};
window.addEventListener('WebComponentsReady', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the whole demo have to change?

Copy link
Contributor Author

@bicknellr bicknellr Mar 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<iron-meta type="iconset" list="{{iconsets}}"></iron-meta>

.. doesn't work anymore because list is no longer an observable property. Also, having the extra <dom-repeat> around the repeated items kind of screws up styling them since they're meant to flex and display: contents; doesn't actually work yet. The easiest way to handle this seemed to be to generate the DOM manually but I'm not attached to that method if you've got other recommendations. I tried changing it so that the <dom-repeat> elements were used for display instead of wrapping with divs but this seems to trigger a ShadyCSS problem that prevents all the styles on the page from being rewritten properly. I'll keep trying to revert this but the ShadyCSS thing is blocking it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tl;dr of @notwaldorf offline: 'wat, no'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list part makes sense and the var iconsets line makes sense. The constructing a 💩 ton of DOM seems silly and let's maybe not do that :)

document.querySelector('[is=dom-bind]').getIconNames = function(iconset) {
return iconset.getIconNames();
};
window.addEventListener('WebComponentsReady', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The list part makes sense and the var iconsets line makes sense. The constructing a 💩 ton of DOM seems silly and let's maybe not do that :)

@bicknellr
Copy link
Contributor Author

Still trying to figure out a minimal repro for this styling issue in the demo.

@bicknellr
Copy link
Contributor Author

Submitted as Polymer/polymer#4521.

@bicknellr
Copy link
Contributor Author

Polymer/polymer#4521 is fixed; I'll try reverting the demo changes again.

@bicknellr
Copy link
Contributor Author

Oh, there's not a release yet, the fix is still only available in master.

@bicknellr
Copy link
Contributor Author

bicknellr commented Apr 11, 2017

Updating your local copy of Polymer to master makes the demo work but there's no release, so it won't work with just polymer install --variants yet. :(

(edit: weird grammar)

demo/index.html Outdated
return iconset.getIconNames();
};
window.addEventListener('WebComponentsReady', function() {
scope.getIconNames =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this trailing = is intentional, right? i might move the scope.parentNode line below to make that explicit, or indent it by 4. or something. right now it loos like a typo :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, intentional. I changed this to two assignments.

@notwaldorf
Copy link
Contributor

@bicknellr one small nit left. gonna keep this red though until we can verify it works with the next rc release (hopefully tomorrow)

@bicknellr
Copy link
Contributor Author

@notwaldorf, 2.0.0-rc.4 is out with the custom-style fix.

Copy link
Contributor

@notwaldorf notwaldorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌

@bicknellr bicknellr merged commit 5e6b568 into master May 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants