Add renderer

[yuriy-fix]

Fixes #76

---

This change is 

[manolo]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @YuriyVaadin)

---

src/vaadin-notification.html, line 261 at r1 (raw file):

             * `root` The root container element. Users are able to append content to it.
             *
             * `model` Object with properties that could be required during the rendering process.

It is odd that the renderer can take an argument (model) that cannot be set. Actually it's always null in this case.

---

src/vaadin-notification.html, line 263 at r1 (raw file):

             * `model` Object with properties that could be required during the rendering process.
             *
             * `owner` The host element of the renderer function.

owner is always this._card like this._root is, not sure if we should call the renderer with these parameters. Anyways seems confussing the API

[coveralls]

Pull Request Test Coverage Report for Build 485

• 43 of 43 (100.0%) changed or added relevant lines in 1 file are covered.
• 11 unchanged lines in 1 file lost coverage.
• Overall coverage decreased (-8.8%) to 88.83%

---

Files with Coverage Reduction	New Missed Lines	%
src/vaadin-notification.html	11	88.83%

Totals
Change from base Build 463:	-8.8%
Covered Lines:	105
Relevant Lines:	116

---

💛 - Coveralls

[yuriy-fix]

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @YuriyVaadin)

---

src/vaadin-notification.html, line 261 at r1 (raw file):

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

It is odd that the renderer can take an argument (model) that cannot be set. Actually it's always null in this case.

Model is dropped now. Was added to align with other components.

---

src/vaadin-notification.html, line 263 at r1 (raw file):

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

owner is always this._card like this._root is, not sure if we should call the renderer with these parameters. Anyways seems confussing the API

The notification itself is now passed as owner

[manolo]

Apparently coverage decreased, can we increase somehow ?

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

---

src/vaadin-notification.html, line 261 at r1 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Model is dropped now. Was added to align with other components.

But we should maintain the parameter, so as user might eventually reuse renderers between components.
We might document that it's always null because notification does not need any model

---

src/vaadin-notification.html, line 263 at r2 (raw file):

             * `owner` The host element of the renderer function.
             *
             * **NOTE:** The renderer callback can be called several times with previous content available

remove available, it's confusing in the sentence and it's not needed

---

src/vaadin-notification.html, line 395 at r2 (raw file):

          if (renderer) {
            this.renderer(this._card, this);

if we remove the second parameter we misalign this renderer with other elements renderers

[tomivirkki]

---

src/vaadin-notification.html, line 395 at r2 (raw file):

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

if we remove the second parameter we misalign this renderer with other elements renderers

Should the order of the arguments be reconsidered since "model" is not used everywhere?

[yuriy-fix]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @YuriyVaadin and @manolo)

---

src/vaadin-notification.html, line 395 at r2 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

Should the order of the arguments be reconsidered since "model" is not used everywhere?

I guess we can set it to root, owner, model. Where the model is not provided in all of the components

[yuriy-fix]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @YuriyVaadin and @manolo)

---

src/vaadin-notification.html, line 263 at r2 (raw file):

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

remove available, it's confusing in the sentence and it's not needed

Should I totally remove it or change to when available? It's for the cases when the renderer is redefined or defined after the template. In those cases the root is cleaned and user won't receive any content

[yuriy-fix]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @YuriyVaadin and @manolo)

---

src/vaadin-notification.html, line 261 at r1 (raw file):

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

But we should maintain the parameter, so as user might eventually reuse renderers between components.
We might document that it's always null because notification does not need any model

Updated the docs.

[tomivirkki]

---

demo/notification-basic-demos.html, line 40 at r3 (raw file):

    <vaadin-demo-snippet id="notification-basic-demos-renderer-notification">
      <template preserve-content>
        <dom-bind id="bind-elm">

The renderer feature is a step towards reducing the users' necessity to use Polymer's proprietary APIs (so that the components would be more JS/framework user friendly). Using Polymer's "dom-bind + template" with Polymer-syntax bindings in the renderer demo doesn't serve the purpose.

[tomivirkki]

---

src/vaadin-notification.html, line 257 at r3 (raw file):

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

Why isn't "model" mentioned in this list but the argument is still documented below?

[manolo]

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

---

src/vaadin-notification.html, line 263 at r2 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Should I totally remove it or change to when available? It's for the cases when the renderer is redefined or defined after the template. In those cases the root is cleaned and user won't receive any content

To me available means a different thing, probably the correct sentence might be

The renderer callback can be called multiple times or The renderer callback can be called multiple times with the previous content

---

src/vaadin-notification.html, line 263 at r3 (raw file):

             * `owner` The host element of the renderer function.
             *
             * `model` null

I guess we can remove model it would confuse audience that document a parameter that it's always null.

[yuriy-fix]

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @YuriyVaadin)

---

demo/notification-basic-demos.html, line 40 at r3 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

The renderer feature is a step towards reducing the users' necessity to use Polymer's proprietary APIs (so that the components would be more JS/framework user friendly). Using Polymer's "dom-bind + template" with Polymer-syntax bindings in the renderer demo doesn't serve the purpose.

Updated and tested. Moved for proper checking on IE11

---

src/vaadin-notification.html, line 263 at r2 (raw file):

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

To me available means a different thing, probably the correct sentence might be

The renderer callback can be called multiple times or The renderer callback can be called multiple times with the previous content

Updated.

---

src/vaadin-notification.html, line 257 at r3 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

Why isn't "model" mentioned in this list but the argument is still documented below?

Listed now and docs aligned with other components.

---

src/vaadin-notification.html, line 263 at r3 (raw file):

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

I guess we can remove model it would confuse audience that document a parameter that it's always null.

Included to be aligned with other components.

[sohrabtaee]

---

demo/notification-basic-demos.html, line 83 at r4 (raw file):

        </script>
      </template>
    </vaadin-demo-snippet>saf

What is this "saf" after the closing tag?

[yuriy-fix]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @manolo, @tomivirkki, and @YuriyVaadin)

---

demo/notification-basic-demos.html, line 83 at r4 (raw file):

Previously, sohrabtaee (Sohrab Taee) wrote…

What is this "saf" after the closing tag?

Accidentally pushed with latest commit. Removed.

[platosha]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @manolo, @tomivirkki, and @YuriyVaadin)

---

src/vaadin-notification.html, line 263 at r3 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Included to be aligned with other components.

+1 do remove. Actually it’s undefined in the overlay ATM.

---

src/vaadin-notification.html, line 257 at r4 (raw file):

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

notification instead of owner, please

[tomivirkki]

---

src/vaadin-notification.html, line 273 at r5 (raw file):

             * The template of the notification card content.
             */
            template: Object

Why is this new public API added?!!!

[yuriy-fix]

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

---

src/vaadin-notification.html, line 263 at r3 (raw file):

Previously, platosha (Anton Platonov) wrote…

+1 do remove. Actually it’s undefined in the overlay ATM.

Done.

---

src/vaadin-notification.html, line 257 at r4 (raw file):

Previously, platosha (Anton Platonov) wrote…

notification instead of owner, please

Done.

---

src/vaadin-notification.html, line 273 at r5 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

Why is this new public API added?!!!

Changed.

[platosha]

Reviewed 1 of 4 files at r1, 1 of 4 files at r4, 2 of 4 files at r5, 3 of 3 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @manolo and @tomivirkki)

---

src/vaadin-notification.html, line 257 at r3 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Listed now and docs aligned with other components.

Let’s remove model from this line since it’s removed from below.

[manolo]

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

---

src/vaadin-notification.html, line 263 at r3 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Included to be aligned with other components.

ok

---

test/notification-renderer-test.html, line 109 at r6 (raw file):

        });

        it('the initial template instance should be used after renderer was assigned and removed', () => {

Is this a wanted feature? in grid we removed it. Ping @tomivirkki

[yuriy-fix]

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

---

src/vaadin-notification.html, line 257 at r3 (raw file):

Previously, platosha (Anton Platonov) wrote…

Let’s remove model from this line since it’s removed from below.

Removed.

---

test/notification-renderer-test.html, line 109 at r6 (raw file):

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

Is this a wanted feature? in grid we removed it. Ping @tomivirkki

Should I remove it in scope of this PR?

[tomivirkki]

---

test/notification-renderer-test.html, line 109 at r6 (raw file):

Previously, YuriyVaadin (Yuriy Yevstihnyeyev) wrote…

Should I remove it in scope of this PR?

We kind of agreed yesterday that it can be removed separately if it's already implemented (if the PR author prefers that).

[manolo]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tomivirkki)

---

test/notification-renderer-test.html, line 109 at r6 (raw file):

Previously, tomivirkki (Tomi Virkki) wrote…

We kind of agreed yesterday that it can be removed separately if it's already implemented (if the PR author prefers that).

Perfect, let's do in separated PR

[manolo]

Reviewed 1 of 4 files at r1, 1 of 4 files at r4, 1 of 4 files at r5, 3 of 3 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tomivirkki)

[manolo]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @tomivirkki)

[tomivirkki]

Reviewed 1 of 4 files at r1, 1 of 4 files at r4, 1 of 4 files at r5, 3 of 3 files at r6.
Reviewable status: complete! all files reviewed, all discussions resolved