Initial reviewable code #1
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| language: node_js | ||
| sudo: required | ||
| dist: trusty | ||
| before_script: | ||
| - npm install web-component-tester | ||
| - npm install bower | ||
| - 'export PATH=$PWD/node_modules/.bin:$PATH' | ||
| - bower install | ||
| node_js: 6 | ||
| addons: | ||
| apt: | ||
| sources: | ||
| - google-chrome | ||
| packages: | ||
| - google-chrome-stable | ||
| script: | ||
| - xvfb-run wct | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # <palindrom-error-catcher> [](https://travis-ci.org/Palindrom/palindrom-error-catcher) | ||
|
|
||
| --- | ||
|
|
||
| > Handles palindrom-client disconnection events and creates the needed UI to give the user control over them | ||
|
|
||
| Custom Element that binds with [palindrom-client](https://github.com/Palindrom/palindrom-client) connection events and shows a simple UX that allows the user to interact with them. It is can be used as an example of designing your own error catcher. | ||
|
There was a problem hiding this comment. "shows UI", I think experience is not the thing to show, is a thing to feel. |
||
|
|
||
| Please check the code at `palindrom-error-catcher.html` file to see how events are handles. | ||
|
|
||
|
|
||
| # Creating your own | ||
|
|
||
| If you want to gain control over the appearance of your errors UI. You can fork this element, put it in the `wwwroot/sys/` folder of your app. And edit it as desired. Once you have an element in this folder with the same name, it will supersede the default one and your UI will be shows. | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| { | ||
| "name": "palindrom-error-catcher", | ||
| "version": "1.0.0", | ||
| "description": | ||
| "Handles palindrom-client disconnection events and creates the needed UI to give the user control over them", | ||
| "private": true, | ||
| "authors": ["Omar Alshaker"], | ||
| "keywords": [ | ||
| "web-components", | ||
| "polymer", | ||
| "json-patch", | ||
| "palindrom", | ||
| "binding", | ||
| "puppetjs", | ||
| "template", | ||
| "web-socket", | ||
| "http", | ||
| "json" | ||
| ], | ||
| "main": "palindrom-error-catcher.html", | ||
| "repository": { | ||
| "type": "git", | ||
| "url": "git://github.com/Palindrom/palindrom-error-catcher.git" | ||
| }, | ||
| "license": "MIT", | ||
| "homepage": "https://github.com/Palindrom/palindrom-error-catcher", | ||
| "ignore": ["**/.*", "node_modules", "bower_components", "test/"], | ||
| "devDependencies": { | ||
| "test-fixture": "PolymerElements/test-fixture#^3.0.0", | ||
| "web-component-tester": "^6.0.0", | ||
| "webcomponentsjs": "webcomponents/webcomponentsjs#^1.0.0" | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <!doctype html> | ||
| <html> | ||
|
|
||
| <head> | ||
| <meta http-equiv="refresh" content="0;URL='/components/palindrom-error-catcher/test/" /> | ||
| </head> | ||
|
|
||
| </html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,170 @@ | ||
| <!-- palindrom-error-catcher version: 1.0.0 | MIT License --> | ||
|
|
||
| <!-- | ||
| Custom Element that binds with [palindrom-client](https://github.com/Palindrom/palindrom-client) connection events and shows a simple UX that allows the user to interact with them. It is can be used as an example of designing your own error catcher. | ||
| --> | ||
|
|
||
| <template id="palindrom-error-catcher"> | ||
| <style> | ||
| .box-container { | ||
| position: fixed; | ||
| top: 0; | ||
| left: 0; | ||
| width: 100%; | ||
| z-index: 1000; | ||
| } | ||
|
|
||
| .box-container[hidden] { | ||
| display: none; | ||
| } | ||
|
|
||
| .box { | ||
| padding: 5px 5px; | ||
| background-color: #fee; | ||
| border: 1px solid #f9c1c1; | ||
| border-top: none; | ||
| color: #444; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .hourglass { | ||
| animation: rotateHG 2s cubic-bezier(.03, 1, 1, 0) infinite; | ||
| transform-origin: 50% 50%; | ||
| display: inline-block; | ||
| text-decoration: none !important; | ||
| } | ||
|
|
||
| @keyframes rotateHG { | ||
| 100% { | ||
| transform: rotate(360deg) | ||
| } | ||
| } | ||
| </style> | ||
| <div class="box-container connection-error-pane" hidden> | ||
| <div class="box error tappable"> | ||
| <span class="hourglass">⏳</span> Connection error.</strong> See console for details. | ||
| <a class="reloading-in-message"> | ||
| Reloading page in | ||
| <span class="reconnection-seconds">5</span>... • | ||
| <a href="javascript:;" class="cancel-reloading-btn">Do not reload</a> • | ||
| </a> | ||
| <a href="javascript:;" class="reload-btn">Reload now</a> | ||
| </div> | ||
| </div> | ||
| <div class="box-container generic-error-pane" hidden> | ||
| <div class="box error tappable"> | ||
| <strong>⚠️ An error has occurred.</strong> See console for details. | ||
| <span>Click here to reload</span> | ||
| </div> | ||
| </div> | ||
| </template> | ||
|
|
||
| <script> | ||
| (function () { | ||
| const template = document.currentScript.ownerDocument.querySelector('#palindrom-error-catcher'); | ||
|
|
||
| class PalindromErrorCatcher extends HTMLElement { | ||
| static get observedAttributes() { | ||
| return ['target-selector'] | ||
| } | ||
| constructor() { | ||
| super(); | ||
|
|
||
| // Creates the shadow root | ||
| const shadowRoot = this.attachShadow({ mode: 'open' }); | ||
| const clone = document.importNode(template.content, true); | ||
| shadowRoot.appendChild(clone); | ||
|
|
||
| this.connectionErrorPane = shadowRoot.querySelector('.connection-error-pane'); | ||
| this.genericErrorPane = shadowRoot.querySelector('.generic-error-pane'); | ||
| this.reconnectionSecondsSpan = shadowRoot.querySelector('.reconnection-seconds'); | ||
| this.cancelReloadingBtn = shadowRoot.querySelector('.cancel-reloading-btn'); | ||
| this.reloadNowBtn = shadowRoot.querySelector('.reload-btn'); | ||
|
|
||
| this.targetSelector = this.getAttribute('target-selector') || 'palindrom-client'; | ||
|
There was a problem hiding this comment. You should not read attributes in the constructor |
||
| this._bindCallbacks(); | ||
| } | ||
| _bindToElement() { | ||
| this.target = this.getRootNode().querySelector(this.targetSelector); | ||
|
There was a problem hiding this comment. I'dcheck in the |
||
| } | ||
| _bindCallbacks() { | ||
|
|
||
| this.handleConnectionError = this.handleConnectionError.bind(this); | ||
| this.handleReconnectionCountdownTick = this.handleReconnectionCountdownTick.bind(this); | ||
| this.handleReconnectionEnd = this.handleReconnectionEnd.bind(this); | ||
| this.handleGenericError = this.handleGenericError.bind(this); | ||
| this.cancelReloading = this.cancelReloading.bind(this); | ||
| this._reload = this._reload.bind(this); | ||
| } | ||
| _subscribeToEvents() { | ||
| this._unsubscribeToEvents(); | ||
| this.target.addEventListener('generic-error', this.handleGenericError); | ||
| this.target.addEventListener('connection-error', this.handleConnectionError); | ||
| this.target.addEventListener('reconnection-countdown', this.handleReconnectionCountdownTick); | ||
| this.target.addEventListener('reconnection-end', this.handleReconnectionEnd); | ||
| this.cancelReloadingBtn.addEventListener('click', this.cancelReloading); | ||
| this.reloadNowBtn.addEventListener('click', this._reload); | ||
| this.genericErrorPane.addEventListener('click', this._reload); | ||
| this.subscribed = true; | ||
| } | ||
| _unsubscribeToEvents() { | ||
|
There was a problem hiding this comment. I think those three There was a problem hiding this comment. But this is extremely anti-pattern. They're part of the element class and I feel leaving them outside and passing parameters to them is like making static functions in a class and using them in another class once. This misses the point of the class altogether. I think the underscore should suffice to say that they're not part of the API. I mean I understand doing this for utility helper functions, but these functions are like life-cycle methods for the instance, doesn't feel right to keep them outside. There was a problem hiding this comment. I wouldn't say it's "extremely anti-pattern" it's opinionated, what is worse expose private methods as effectively public or define private methods outside of You can see discussions like this in many places, like: Personally, I think putting variably/function outside of published scope is and was the way to make them local/private in JavaScript since the very beginning, I don't like pretending JavaScript and its classes are like Java. Usually, I try to avoid quasi-/convention-based- private members as much as I can, unless I really need them for inheritance or smth. As I never trus convention to be reliable security guard. There was a problem hiding this comment. Anyway, as I said to me it's just an opinion if you have the different one, go ahead, that was just my comment and suggestion. Failing FF, and lack of contributing are more blocking problems :) |
||
| if (this.subscribed) { | ||
| this.target.removeEventListener('generic-error', this.handleGenericError); | ||
| this.target.removeEventListener('connection-error', this.handleConnectionError); | ||
| this.target.removeEventListener('reconnection-countdown', this.handleReconnectionCountdownTick); | ||
| this.target.removeEventListener('reconnection-end', this.handleReconnectionEnd); | ||
| this.genericErrorPane.removeEventListener('click', this._reload); | ||
| this.cancelReloadingBtn.removeEventListener('click', this.cancelReloading); | ||
| this.reloadNowBtn.removeEventListener('click', this._reload); | ||
| this.subscribed = false; | ||
| } | ||
| } | ||
| _reload() { | ||
| location.reload(); | ||
| } | ||
| connectedCallback() { | ||
| this._bindToElement(); | ||
| this._subscribeToEvents(); | ||
| } | ||
| disconnectedCallback() { | ||
| this._unsubscribeToEvents(); | ||
| } | ||
| handleGenericError(event) { | ||
| this.genericErrorPane.removeAttribute('hidden'); | ||
| console.error(event.detail.error); | ||
| } | ||
| cancelReloading() { | ||
| this.connectionErrorPane.setAttribute('hidden', ''); | ||
| clearInterval(this.reloadInterval); | ||
| delete this.reloadInterval; | ||
| } | ||
| handleConnectionError(event) { | ||
| this.connectionErrorPane.removeAttribute('hidden'); | ||
| let timeout = 4; | ||
| // in case two consequtive errors happen, we don't want to schedule reloading twice | ||
| if (!this.reloadInterval) { | ||
| this.reloadInterval = setInterval(() => { | ||
| this.reconnectionSecondsSpan.textContent = timeout--; | ||
| if (timeout === 0) { | ||
| this._reload(); | ||
| } | ||
| }, 1000); | ||
| } | ||
| console.error(event.detail.error); | ||
| } | ||
| handleReconnectionCountdownTick(event) { | ||
| /* you can use this method to add your logic to handle reconnection timer ticks */ | ||
| console.log('Reconnection tick', event) | ||
| } | ||
| handleReconnectionEnd() { | ||
| /* you can use this method to add your logic to handle reconnection initiation. */ | ||
| console.log('Reconnection will happen now...') | ||
| } | ||
| attributeChangedCallback(name, oldVal, newVal) { | ||
| this.targetSelector = newVal; | ||
| this._bindToElement(); | ||
| this._subscribeToEvents(); | ||
| } | ||
| } | ||
| customElements.define('palindrom-error-catcher', PalindromErrorCatcher) | ||
| })(); | ||
| </script> | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| <html> | ||
|
|
||
| <head> | ||
| <meta charset="utf-8"> | ||
|
|
||
| <!-- Importing Web Component's Polyfill --> | ||
| <script src="../../../webcomponentsjs/webcomponents-lite.js"></script> | ||
|
|
||
| <script src="../../../sinonjs/sinon.js"></script> | ||
|
|
||
| <link rel="import" href="../shared/helpers.html"> | ||
|
|
||
| <script src="../../../web-component-tester/browser.js"></script> | ||
|
|
||
| <!-- Step 1: import the element to test --> | ||
| <link rel="import" href="../../palindrom-error-catcher.html"> | ||
| </head> | ||
|
|
||
| <body> | ||
| <test-fixture id="text-fixture"> | ||
| <template> | ||
| <palindrom-client></palindrom-client> | ||
| <palindrom-error-catcher></palindrom-error-catcher> | ||
| </template> | ||
| </test-fixture> | ||
| <script> | ||
| describe('palindrom-error-catcher', function () { | ||
| var textFixture, ppclient, palindromErrorCatcher, errorCatcherShadowRoot; | ||
| before(function () { | ||
| chai.use(palindromChaiPlugin); | ||
| }); | ||
| beforeEach(function () { | ||
| textFixture = fixture('text-fixture'); | ||
| ppclient = textFixture[0]; | ||
| palindromErrorCatcher = textFixture[1]; | ||
| errorCatcherShadowRoot = palindromErrorCatcher.shadowRoot; | ||
| }); | ||
| describe('Connection errors: UI', function () { | ||
| it('Should show UI after connection error', function () { | ||
| const errorPane = errorCatcherShadowRoot.querySelector('.connection-error-pane'); | ||
|
|
||
| expect(errorPane).to.be.not.visible; | ||
| ppclient.fireConnectionErrorEvent(); | ||
| expect(errorPane).to.be.visible; | ||
| }); | ||
| it('Should show UI after generic error', function () { | ||
| const errorPane = errorCatcherShadowRoot.querySelector('.generic-error-pane'); | ||
|
|
||
| expect(errorPane).to.be.not.visible; | ||
| ppclient.fireGenericErrorEvent(); | ||
| expect(errorPane).to.be.visible; | ||
| }); | ||
| }); | ||
| describe('Connection errors: console', function () { | ||
| var consoleErrorSpy; | ||
|
|
||
| beforeEach(function () { | ||
| consoleErrorSpy = sinon.spy(console, 'error'); | ||
| }); | ||
| afterEach(function () { | ||
| console.error.restore(); | ||
| }); | ||
|
|
||
| it('Should call console.error after connection error', function () { | ||
| expect(consoleErrorSpy).to.be.not.called; | ||
| ppclient.fireConnectionErrorEvent('some connection error'); | ||
| expect(consoleErrorSpy).to.be.called; | ||
| expect(consoleErrorSpy.getCall(0).args[0].message).to.contain('some connection error'); | ||
| }); | ||
| it('Should call console.error after generic error', function () { | ||
| expect(consoleErrorSpy).to.be.not.called; | ||
| ppclient.fireGenericErrorEvent('some generic error'); | ||
| expect(consoleErrorSpy).to.be.called; | ||
| expect(consoleErrorSpy.getCall(0).args[0].message).to.contain('some generic error'); | ||
| }); | ||
| }); | ||
| }); | ||
| </script> | ||
|
|
||
| </body> | ||
|
|
||
| </html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| <!DOCTYPE html> | ||
| <html><head> | ||
| <meta charset="utf-8"> | ||
| <script src="../../webcomponentsjs/webcomponents-lite.js"></script> | ||
| <script src="../../web-component-tester/browser.js"></script> | ||
| </head> | ||
| <body> | ||
| <script> | ||
| WCT.loadSuites([ | ||
| 'd-b-n_smoke/simple.html' | ||
| ]); | ||
| </script> | ||
|
|
||
|
|
||
| </body></html> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| <script> | ||
| window.palindromChaiPlugin = function (chai, utils) { | ||
| chai.Assertion.addProperty('visible', function () { | ||
| this.assert(this._obj.offsetWidth || this._obj.offsetHeight || this._obj.getClientRects().length, | ||
| "expected element #{this} to be visible, but it's hidden", | ||
| "expected element #{this} to be hidden, but it's visible"); | ||
| }) | ||
| }; | ||
| customElements.define('palindrom-client', class PalindromClientMock extends HTMLElement { | ||
| fireConnectionErrorEvent(msg) { | ||
| this.dispatchEvent(new CustomEvent('connection-error', { | ||
| detail: { error: new Error(msg) } | ||
| })); | ||
| } | ||
| fireGenericErrorEvent(msg) { | ||
| this.dispatchEvent(new CustomEvent('generic-error', { | ||
| detail: { error: new Error(msg) } | ||
| })); | ||
| } | ||
| fireReconnectionTickEvent() { | ||
| this.dispatchEvent(new CustomEvent('reconnection-countdown')); | ||
| } | ||
| fireReconnectionEndEvent() { | ||
| this.dispatchEvent(new CustomEvent('reconnection-end')); | ||
| } | ||
| }); | ||
| </script> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| { | ||
| "verbose": false, | ||
| "trackConsoleError": false, | ||
| "plugins": { | ||
| "local": { | ||
| "browsers": ["chrome"] | ||
| }, | ||
| "sauce": { | ||
| "disabled": false, | ||
| "browsers": [{ | ||
| "browserName": "firefox", | ||
|
There was a problem hiding this comment. I think we should test in Edge as well. Plus we should be able to test firefox in |
||
| "platform": "Windows 10" | ||
| }, | ||
| { | ||
| "browserName": "safari", | ||
| "platform": "macOS 10.13" | ||
| }] | ||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use 8 to be more up to dane.