search

dart-archive / polymer_elements

https://pub.dartlang.org/packages/polymer_elements
BSD 3-Clause "New" or "Revised" License
24 stars 17 forks source link

upgrading paper_elements to 1.0.6 #109

Closed dam0vm3nt closed 8 years ago

dam0vm3nt commented 8 years ago

This should fix #93 even though there are still problems with paper-dialog.

jakemac53 commented 8 years ago

As a part of the upgrade we also need to update the tests. Are you willing to do that or should we merge this into a separate branch and then I can work on doing it?

jakemac53 commented 8 years ago

Also I am not sure if you have used reitveld before, but for reviews of this size that is typically what we use just because it can handle unlimited numbers of files. That is not a requirement (I can just review the changes locally) but if you already have it set up that would be great.

dam0vm3nt commented 8 years ago

I can update the test, but I'm not sure I understand what this actually means (e.g. : adding tests for new elements ?).

Never used reitveld, sorry, but it sounds interesting I'll check it.

But if this can help reviewing basically I've only updated the version of paper_elements in bower.json, added generation of polymer_spinner_style.dart in polymer_elements_config.yaml and run the custom_element_apigen tool. The big changes are due to the paper_element updates from JS.

jakemac53 commented 8 years ago

I can update the test, but I'm not sure I understand what this actually means (e.g. : adding tests for new elements ?).

We have ported all the js tests to dart, and so we want to maintain those. Basically you just look at the diffs for each of the js tests, and then go apply those same changes to the dart tests.

Never used reitveld, sorry, but it sounds interesting I'll check it.

https://github.com/dart-lang/sdk/wiki/Code-review-workflow-with-GitHub-and-reitveld is some docs on this, but its a bit of work to get it all set up. If you have some other app locally that you use to view diffs then that might be easier :).

But if this can help reviewing basically I've only updated the version of paper_elements in bower.json, added generation of polymer_spinner_style.dart in polymer_elements_config.yaml and run the custom_element_apigen tool. The big changes are due to the paper_element updates from JS.

I still have to actually look over the changes just to make sure haha :)

dam0vm3nt commented 8 years ago

ok, I'm going to port tests too and I'll take a look at reitveld.

In the meanwhile do you have an idea why polymer-dart is not working well (on firefox/safari) with external imported stylesheets? (I think that is the actual cause of #93 ), see here for more info.

jakemac53 commented 8 years ago

I think the issue is that the web_components transformer specifically skips them (on purpose). This was done to prevent duplicate styles getting inlined in multiple elements (the typical reason for using an external stylesheet is if its shared).

Luckily these are already deprecated anyways, so I don't have any plans currently to try and actually address the issue and figure out why not inlining them is causing issues in FF. The elements should start migrating off of them soon.

dam0vm3nt commented 8 years ago

OK, good to know this. So basically we just have to wait for elements to update.

Maybe then it is better to wait for them and abort this upgrade ?

jakemac53 commented 8 years ago

Well some of the elements have already moved off of it (like paper-drawer-panel) so it definitely has value, its up to you. Also generally updating the elements is always good.

dam0vm3nt commented 8 years ago

Ok. Just didn't want to burden you with having to revise something twice. I'll proceed also because I really can't stand that bug anymore :-)

dam0vm3nt commented 8 years ago

@jakemac53 I think I've finished porting the tests but I did not succeed in setting up the reitveld workflow (specifically cannot find how to install git-cl). Hope this is enough though if you want to take a look at it.

Since there were still problems with paper-header-panel and paper-dialog styles on FF I have patched 'em in another branch of mine. If you think is worth instead of waiting for polymers to fix 'em let me know and I will PR it.

jakemac53 commented 8 years ago

I can just pull this down locally and diff it

jakemac53 commented 8 years ago

Seeing some failing tests, can you look into those? It uses the regular test runner - pub run test -pdartium. The iron-collapse test is flaky but the others should succeed.

dam0vm3nt commented 8 years ago

Yep I will.

Il giorno lun 16 nov 2015, 21:25 Jacob MacDonald notifications@github.com ha scritto:

Seeing some failing tests, can you look into those? It uses the regular test runner - pub run test -pdartium. The iron-collapse test is flaky but the others should succeed.

— Reply to this email directly or view it on GitHub https://github.com/dart-lang/polymer_elements/pull/109#issuecomment-157159283 .

dam0vm3nt commented 8 years ago

Fixed some tests but pretty stuck on the remaining. @jakemac53 any hint ?

jakemac53 commented 8 years ago

Well, it looks like at least for paper-radio-group the original js tests are failing as well :/ repo, travis

jakemac53 commented 8 years ago

I tried messing around a bit with the paper-radio-group and it looks like possibly some things are happening async that used to be sync, so adding in a few await wait(0); calls will likely resolve some of the failing tests.

dam0vm3nt commented 8 years ago

added await wait(0); and now they work, I'll try to fix the remaining but I fear I will be very slow at it.

dam0vm3nt commented 8 years ago

marked transitive test on iron-a11-keys-behavior as skipped because original JS test was failing. Maybe upgrade that element to the last version ?

iron-form reset of field does not work. I see that _customElements get their value resetted but that change does not get propagated to dart methink ...

what do you think about this @jakemac53 ?