manolo / gwt-api-generator

Generator for creating GWT JSInterop clients from Polymer Web Components
Apache License 2.0
50 stars 24 forks source link

Generated discaimers contain information about the related component #14

Closed tomivirkki closed 9 years ago

tomivirkki commented 9 years ago

Review on Reviewable

manolo commented 9 years ago

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


gulpfile.js, line 132 [r1] (raw file): I would get rid of bowerData parameter since it comes in obj.bowerData


template/Element.template, line 1 [r1] (raw file): Use just disclaimer() since helper is executed in the item context


template/helpers.js, line 188 [r1] (raw file): this.bowerData should bring this info


template/ImportsMap.template, line 2 [r1] (raw file): This file was removed, why do you commit again?


Comments from the review on Reviewable.io

tomivirkki commented 9 years ago

gulpfile.js, line 132 [r1] (raw file): In the task "generate:events" it doesn't because event != item. Maybe we could set event.bowerData = item.bowerData instead.


Comments from the review on Reviewable.io

tomivirkki commented 9 years ago

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


gulpfile.js, line 132 [r1] (raw file): Done.


template/Element.template, line 1 [r1] (raw file): Done.


template/helpers.js, line 188 [r1] (raw file): Done.


template/ImportsMap.template, line 2 [r1] (raw file): Done.


Comments from the review on Reviewable.io

manolo commented 9 years ago

Reviewed 8 of 8 files at r2. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

Saulis commented 9 years ago

Not sure if this is a deal breaker, but in some cases assigning the item.bowerData to event.bowerData might lead into some confusing results like this:

/*
 * This code was generated with Vaadin Web Component GWT API Generator, 
 * from paper-toggle-button project by The Polymer Authors
 * that is licensed with http://polymer.github.io/LICENSE.txt license.
 */
package com.vaadin.polymer.iron.element.event;

import com.vaadin.polymer.elemental.*;
import com.google.gwt.core.client.JavaScriptObject;
import com.google.gwt.core.client.js.JsProperty;
import com.google.gwt.core.client.js.JsType;

/**
 * <p>Fired when the checked state changes.</p>
 */
@JsType
public interface IronChangeEvent extends Event {

    static final String NAME = "iron-change";

    public abstract class Listener implements EventListener {
        protected abstract void handleEvent(IronChangeEvent event);

        @Override
        public void handleEvent(Event event) {
            handleEvent((IronChangeEvent) event);
        }
    }
}

Reviewed 8 of 8 files at r2. Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io