Added renderer function

[sohrabtaee]

Fixes #87

---

This change is 

[CLAassistant]

All committers have signed the CLA.

[manolo]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @sohrabtaee, @platosha, and @YuriyVaadin)

---

src/vaadin-dialog.html, line 128 at r1 (raw file):

            * Custom function for rendering the content of the overlay
            */
            renderer: Function,

I feel that dialog user should be able to configure the model for the overlay, probably we need to expose model in the component

[sohrabtaee]

---

src/vaadin-dialog.html, line 128 at r1 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

I feel that dialog user should be able to configure the model for the overlay, probably we need to expose model in the component

We didn't find a use case for it. Do you have a use case for exposing model?

[manolo]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @manolo, @platosha, and @YuriyVaadin)

---

src/vaadin-dialog.html, line 128 at r1 (raw file):

Previously, sohrabtaee (Sohrab Taee) wrote…

We didn't find a use case for it. Do you have a use case for exposing model?

When you have a form in a dialog e.g. Product, developer might want to change the product being showed, then overlay needs to re-run renderer.

[sohrabtaee]

---

src/vaadin-dialog.html, line 128 at r1 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

When you have a form in a dialog e.g. Product, developer might want to change the product being showed, then overlay needs to re-run renderer.

After discussing it with the team, we decided that it's not needed and can be implemented as a separate feature later.

[manolo]

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @platosha and @YuriyVaadin)

---

src/vaadin-dialog.html, line 128 at r1 (raw file):

Previously, sohrabtaee (Sohrab Taee) wrote…

After discussing it with the team, we decided that it's not needed and can be implemented as a separate feature later.

ok

[yuriy-fix]

Reviewed 4 of 4 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sohrabtaee and @platosha)

---

demo/dialog-basic-demos.html, line 44 at r1 (raw file):

              const paragraph = window.document.createElement('p');
              const btn = window.document.createElement('vaadin-button');
              paragraph.textContent = 'The content of this overlay was generated with renderer';

NIT: Could be better to use The content of the dialog.
Not a blocker.

---

src/vaadin-dialog.html, line 128 at r1 (raw file):

            * Custom function for rendering the content of the overlay
            */
            renderer: Function,

Probably we can expand the docs by including args. Like here: https://github.com/vaadin/vaadin-overlay/pull/96/files#diff-fb13f916817cf30ef14c045caea0e84bR238

---

test/vaadin-dialog_test.html, line 148 at r1 (raw file):

      it('dialog should bind parent property', () => {
        container.message = 'foo';
        expect(overlay.content.querySelector('span').textContent.trim()).to.equal('foo');

Why those tests need to be changed?

---

test/vaadin-dialog_test.html, line 159 at r1 (raw file):

    });

    describe('renderer function', () => {

Probably it's a good idea to create separate test suite (file) like renderer.html.
Not a blocker.

[manolo]

Update bower.json to depend on vaadin-overlay#renderer-preview

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @sohrabtaee and @platosha)

[manolo]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @sohrabtaee and @platosha)

---

src/vaadin-dialog.html, line 127 at r1 (raw file):

            /**
            * Custom function for rendering the content of the overlay
            */

NIT: jsdoc format needs a space more of indentation but the first line

[sohrabtaee]

---

test/vaadin-dialog_test.html, line 148 at r1 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Why those tests need to be changed?

Anton mentioned that it's better to use overlay's native API rather than complex selectors.

[sohrabtaee]

Reviewable status: 1 of 6 files reviewed, 3 unresolved discussions (waiting on @YuriyVaadin and @platosha)

---

src/vaadin-dialog.html, line 128 at r1 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Probably we can expand the docs by including args. Like here: https://github.com/vaadin/vaadin-overlay/pull/96/files#diff-fb13f916817cf30ef14c045caea0e84bR238

Done.

---

test/vaadin-dialog_test.html, line 148 at r1 (raw file):

Previously, sohrabtaee (Sohrab Taee) wrote…

Anton mentioned that it's better to use overlay's native API rather than complex selectors.

Done.

---

test/vaadin-dialog_test.html, line 159 at r1 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Probably it's a good idea to create separate test suite (file) like renderer.html.
Not a blocker.

Done.

[platosha]

Reviewed 1 of 4 files at r1, 5 of 5 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @YuriyVaadin and @sohrabtaee)

---

src/vaadin-dialog.html, line 127 at r2 (raw file):

            /**
             * Custom function for rendering the content of the dialog.
             * Receives arguments `root`, `owner`, `model`

The order of the arguments is wrong. It is root, model, and dialog.

Nit: please use the dialog name for the third argument, makes more sense here.

[tomivirkki]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @YuriyVaadin and @sohrabtaee)

---

demo/dialog-basic-demos.html, line 29 at r2 (raw file):

    </vaadin-demo-snippet>

    <h3>Dialog with Renderer Function</h3>

Make sure the title is aligned with the rest of the renderer PR's

---

test/vaadin-dialog_renderer-test.html, line 31 at r2 (raw file):

      beforeEach(() => {
        dialog = fixture('renderer-function');
        overlay = dialog.$.overlay;

You're only using the content of the overlay, so why not store "overlayContent" here instead of the "overlay"?

---

test/vaadin-dialog_renderer-test.html, line 41 at r2 (raw file):

      it('should render the content of renderer function instead of template content when renderer function provided', () => {
        overlay.renderer = (root, model, dialog) => {

Shouldn't you set the renderer to the dialog, not the overlay?

---

test/vaadin-dialog_renderer-test.html, line 61 at r2 (raw file):

        expect(overlay.content.querySelector('div').textContent).to.include('The content of the dialog');

        overlay.renderer = null;

dialog.renderer

[platosha]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @YuriyVaadin and @sohrabtaee)

---

src/vaadin-dialog.html, line 127 at r2 (raw file):

Previously, platosha (Anton Platonov) wrote…

The order of the arguments is wrong. It is root, model, and dialog.

Nit: please use the dialog name for the third argument, makes more sense here.

OK decided to go with this order

[tomivirkki]

---

demo/dialog-basic-demos.html, line 46 at r2 (raw file):

              paragraph.textContent = 'The content of this overlay was generated with renderer';
              btn.textContent = 'Close';
              btn.addEventListener('click', () => dialog.opened = false);

Make sure that the demo works on IE11 also

[sohrabtaee]

Reviewable status: 4 of 6 files reviewed, 8 unresolved discussions (waiting on @YuriyVaadin, @platosha, @tomivirkki, and @sohrabtaee)

---

demo/dialog-basic-demos.html, line 29 at r2 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

Make sure the title is aligned with the rest of the renderer PR's

Done.

---

demo/dialog-basic-demos.html, line 46 at r2 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

Make sure that the demo works on IE11 also

Done.

---

test/vaadin-dialog_renderer-test.html, line 41 at r2 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

Shouldn't you set the renderer to the dialog, not the overlay?

Done.

---

test/vaadin-dialog_renderer-test.html, line 61 at r2 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

dialog.renderer

Done.

[sohrabtaee]

Reviewable status: 4 of 6 files reviewed, 8 unresolved discussions (waiting on @YuriyVaadin, @platosha, and @tomivirkki)

---

test/vaadin-dialog_renderer-test.html, line 31 at r2 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

You're only using the content of the overlay, so why not store "overlayContent" here instead of the "overlay"?

Done.

[manolo]

Reviewable status: 4 of 6 files reviewed, 9 unresolved discussions (waiting on @YuriyVaadin, @platosha, @tomivirkki, and @sohrabtaee)

---

src/vaadin-dialog.html, line 199 at r3 (raw file):

        _rendererChanged(renderer) {
          this.$.overlay.owner = this;
          this.$.overlay.renderer = renderer;

As already mentioned, you need to depend on rederer-preview version of overlay, please update dependencies

[yuriy-fix]

Reviewable status: 4 of 6 files reviewed, 7 unresolved discussions (waiting on @YuriyVaadin, @platosha, @tomivirkki, and @sohrabtaee)

---

src/vaadin-dialog.html, line 135 at r3 (raw file):

             * `model` null
             *
             * **NOTE:** The renderer callback can be called several times with previous content available

Should be aligned with other components: The renderer callback can be called multiple times with the previous content.

[sohrabtaee]

Reviewable status: 3 of 7 files reviewed, 7 unresolved discussions (waiting on @YuriyVaadin, @platosha, @tomivirkki, and @manolo)

---

src/vaadin-dialog.html, line 135 at r3 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Should be aligned with other components: The renderer callback can be called multiple times with the previous content.

Done.

---

src/vaadin-dialog.html, line 199 at r3 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

As already mentioned, you need to depend on rederer-preview version of overlay, please update dependencies

Done.

[tomivirkki]

The builds are failing, fix

[yuriy-fix]

Reviewed 3 of 5 files at r2, 2 of 2 files at r3, 2 of 2 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tomivirkki and @manolo)

[coveralls]

Pull Request Test Coverage Report for Build 746

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 97.436%

---

Totals
Change from base Build 732:	0.1%
Covered Lines:	27
Relevant Lines:	27

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 755

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 97.436%

---

Totals
Change from base Build 732:	0.1%
Covered Lines:	27
Relevant Lines:	27

---

💛 - Coveralls

[platosha]

Reviewable status: 6 of 8 files reviewed, 7 unresolved discussions (waiting on @YuriyVaadin, @tomivirkki, @manolo, and @sohrabtaee)

---

src/vaadin-dialog.html, line 127 at r6 (raw file):

            /**
             * Custom function for rendering the content of the dialog.
             * Receives arguments `root`, `owner`, `model`

Let us use dialog to name the owner argument.

Let us also drop the model from this list and below, since we anyway never use it in the dialog at the moment.

[sohrabtaee]

Reviewable status: 4 of 8 files reviewed, 7 unresolved discussions (waiting on @YuriyVaadin, @tomivirkki, @manolo, and @platosha)

---

src/vaadin-dialog.html, line 127 at r6 (raw file):

Previously, platosha (Anton Platonov) wrote…

Let us use dialog to name the owner argument.

Let us also drop the model from this list and below, since we anyway never use it in the dialog at the moment.

Done.

[platosha]

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, 2 of 2 files at r7.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @tomivirkki, @manolo, and @sohrabtaee)

---

test/vaadin-dialog_renderer-test.html, line 18 at r7 (raw file):

    <template>
      <vaadin-dialog>
        <template>

Renderer is an alternative to template. Typically users would have either one, not both of them.

The main use case to test here is with a renderer but without a template.

Please add a fixture without any template and use it in the tests, in order to cover the main use case.

[platosha]

Reviewed 1 of 2 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @tomivirkki, @manolo, and @sohrabtaee)

[manolo]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @tomivirkki, @manolo, and @sohrabtaee)

---

.travis.yml, line 17 at r7 (raw file):

  - POLYMER=2 TEST_SUITE=unit_tests
  - POLYMER=2 TEST_SUITE=visual_tests
  - POLYMER=3

why polymer 3 has been removed ?

[tomivirkki]

---

.travis.yml, line 17 at r7 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

why polymer 3 has been removed ?

It's impossible to make the P3 build pass before we actually release something to npm. Once we enter alpha, we'll make the releases and re-enable these

[manolo]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @tomivirkki, @manolo, and @sohrabtaee)

---

.travis.yml, line 17 at r7 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

It's impossible to make the P3 build pass before we actually release something to npm. Once we enter alpha, we'll make the releases and re-enable these

gotcha

[sohrabtaee]

Reviewable status: 7 of 8 files reviewed, 7 unresolved discussions (waiting on @platosha, @tomivirkki, and @manolo)

---

.travis.yml, line 17 at r7 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

It's impossible to make the P3 build pass before we actually release something to npm. Once we enter alpha, we'll make the releases and re-enable these

Done.

---

test/vaadin-dialog_renderer-test.html, line 18 at r7 (raw file):

Previously, platosha (Anton Platonov) wrote…

Renderer is an alternative to template. Typically users would have either one, not both of them.

The main use case to test here is with a renderer but without a template.

Please add a fixture without any template and use it in the tests, in order to cover the main use case.

Done.

[platosha]

Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @tomivirkki, @manolo, and @sohrabtaee)

---

test/vaadin-dialog_renderer-test.html, line 36 at r8 (raw file):

      beforeEach(() => {
        dialog = fixture('renderer-function-with-template');

The title says without, but the fixture is -with-template.

[sohrabtaee]

Reviewable status: 7 of 8 files reviewed, 7 unresolved discussions (waiting on @platosha, @tomivirkki, and @manolo)

---

test/vaadin-dialog_renderer-test.html, line 36 at r8 (raw file):

Previously, platosha (Anton Platonov) wrote…

The title says without, but the fixture is -with-template.

Done.

[platosha]

Reviewed 1 of 1 files at r9.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tomivirkki and @manolo)

[platosha]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @tomivirkki and @manolo)

[tomivirkki]

Reviewed 3 of 5 files at r2, 1 of 2 files at r4, 1 of 1 files at r6, 2 of 2 files at r7, 1 of 1 files at r9.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @manolo)

[manolo]

Reviewable status: complete! all files reviewed, all discussions resolved