-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial reviewable code #1
Merged
Merged
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
0c80a13
Initial reviewable code
alshakero 2a67998
Fix README typos and use Node 7 in Travis
alshakero 5c60097
Address review comments from #1
alshakero a213763
Add Sauce Labs keys
tomalec a469b63
Move simple.html test outside
alshakero 866f690
Change paths in simple.html
alshakero adbdb8e
Fixes
alshakero bca2e1b
Merge branch 'initial-reviewable-code' of https://github.com/Palindro…
alshakero 8af681e
Fix Edge browser name in wct.conf
alshakero 5602c7d
Better paper work
alshakero File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,21 @@ | ||
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: 8 | ||
addons: | ||
apt: | ||
sources: | ||
- google-chrome | ||
packages: | ||
- google-chrome-stable | ||
script: | ||
- xvfb-run wct | ||
env: | ||
global: | ||
- secure: udu+o3NNMHha79ytgmQxCKH99hTEO2egjCPyy97cCMY0b7jkaMM87UtGvrdxJtZGpkSLRPDxIseafZ0Z9Kvn8splH/cNe23m34LuW97BOeI6R9/1HQBJAyG17luVI8wlW2aYq1xD0xJqpxdTyG/hAKB3D3/8KoAIe9r/fH53cNEHi0mfuOqfWuuQmvmrzKD+DFgPj4SmQPLMxBba6O83kmXZw5NIQYYuoEszPmA4wZxtQzNO0DS37g1N7bsoU6505CKzYaXVLApcpCnmz4s5no21TP+tFBPW9zCjul+J+0WR20PxiaGtI1PlnAAdr0LZPUuBxoRpFhoQwZV5ojsTo82JNjexvmEVY2xOLE3+LS1j9IMvK7Fuv1TbV4r9eufAqvYTt4pVVgo6FwuEfLiMx9Tye7GFlfYmJFsv2dSTDekFr1vDi5E28R2V5JVAT/y+q4eSEsgLDSc7Tpts/ME1nUG3wMTH8NaAqZr7opcfa3vBe+dkeXTw8p8DX6lsaUtLiVvpw4hZGG15h0x05proU2e+RBBDp1SX0bovBjzbxSkJklm5Gzc+lVcGWDJtf/szcwLp77+LvKIHHnLMswezF002OlhRPfNQ8xil9bk/GR5PWnrH/n1A8VLYPAaFwU1tf89I4dPhZVnHSj+y8RWkV+SG7FUbbWE4uFBZnX9oQ6I= | ||
- secure: R1ycV0k4ZbLsNrOhCTTBWzA39mYhH5DtDJ+3fUXvfuWKB97dFDBKgsIFCRketQ2y4uLQr9iNA2kMWjygQ1uwL5SscyYypyKCuVT7dAVa6E+wGO4UjxR/8E0VcgmkfHi9wD2neC3GNDumHH5SwZVLHmqSf0QFvB+Gem46ukDuS3wdgTv1+0oGFpQGbruBCTGxWZ2ydWuuC5zWZYoOguy/IZilDU+2wvG5wAFRz5cQuwSIy6iOsprIdtlgvDpBkwSS2KPs16f3j1xntrg995SXfUFDCyVJ3FGusDAAvIYJ25jls4zydUXDH1JrhiD1rhty1Nc3RI2hZ/yFdXll9J0Kl/nPMNpsECn65pn25AfxcPSChjIgXm+U1pcWEQEMGvhvqvVeZL3fp0o8srxtVssmLAQMMDR2vxe3McDrVnKLHEsbRHQEM0GIlRInsMuZ2RvyDpypGMYEkzZvGMVIUCDTS31xOz/Vtr8/WocqaW0wslvg2l4oKsw+ckWWDJbOXOCQ3RyrEl3+qknJ6a5KF7o4TQwxwpxndeEtgeJeHPwHHThV77SWViOtBgBSvmuZwVngVBaerhIUCsOvCdc9xBvHzfz8o94UbuFIPvV9p03yvdplRmIband0bL9tuerHxmNtVq/au2z21WOdsHVrqZEjhrv4XwTkLiw8QUGkPJ44W4I= |
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> [![Build Status](https://travis-ci.org/Palindrom/palindrom-error-catcher.svg?branch=gh-pages)](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 UI that allows the user to interact with the events. It is can be used as an example of designing your own error-preseting UI. | ||
|
||
Please check the code at `palindrom-error-catcher.html` file to see how events are handled. | ||
|
||
# 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,172 @@ | ||
<!-- 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'); | ||
|
||
function bindCallbacks(to) { | ||
to.handleConnectionError = to.handleConnectionError.bind(to); | ||
to.handleReconnectionCountdownTick = to.handleReconnectionCountdownTick.bind(to); | ||
to.handleReconnectionEnd = to.handleReconnectionEnd.bind(to); | ||
to.handleGenericError = to.handleGenericError.bind(to); | ||
to.cancelReloading = to.cancelReloading.bind(to); | ||
} | ||
|
||
function reload() { | ||
location.reload(); | ||
} | ||
|
||
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'); | ||
|
||
bindCallbacks(this); | ||
} | ||
_bindToElement() { | ||
this.target = this.getRootNode().querySelector(this.targetSelector) || document.querySelector(this.targetSelector); | ||
} | ||
|
||
_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', reload); | ||
this.genericErrorPane.addEventListener('click', reload); | ||
this.subscribed = true; | ||
} | ||
_unsubscribeToEvents() { | ||
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', reload); | ||
this.cancelReloadingBtn.removeEventListener('click', this.cancelReloading); | ||
this.reloadNowBtn.removeEventListener('click', reload); | ||
this.subscribed = false; | ||
} | ||
} | ||
connectedCallback() { | ||
this.targetSelector = this.getAttribute('target-selector') || 'palindrom-client'; | ||
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) { | ||
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> |
Oops, something went wrong.
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 think those three
_methods
could also be hidden for public, to make sure nobody will start using them so we will be forced to support it ;)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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
class
keyword. There are just ways to address the problem, see more solutions at http://exploringjs.com/es6/ch_classes.html#sec_private-data-for-classesYou can see discussions like this in many places, like:
airbnb/javascript#1024
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)