Initial reviewable code #1

alshakero · 2018-05-16T11:40:49Z

Related to Palindrom/palindrom-client#71 and Palindrom/palindrom-client#69

tomalec · 2018-05-16T14:02:44Z

.travis.yml

+  - npm install bower
+  - 'export PATH=$PWD/node_modules/.bin:$PATH'
+  - bower install
+node_js: 6


I'd use 8 to be more up to dane.

tomalec · 2018-05-16T14:03:43Z

README.md

+
+> 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.


"shows UI", I think experience is not the thing to show, is a thing to feel.

tomalec · 2018-05-16T14:04:13Z

README.md

+
+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.
+
+Please check the code at `palindrom-error-catcher.html` file to see how events are handles. 


tomalec · 2018-05-16T14:06:18Z

wct.conf.json

+    "sauce": {
+      "disabled": false,
+      "browsers": [{
+        "browserName": "firefox",


I think we should test in Edge as well. Plus we should be able to test firefox in "local"

tomalec · 2018-05-16T14:08:39Z

palindrom-error-catcher.html

+                this.cancelReloadingBtn = shadowRoot.querySelector('.cancel-reloading-btn');
+                this.reloadNowBtn = shadowRoot.querySelector('.reload-btn');
+
+                this.targetSelector = this.getAttribute('target-selector') || 'palindrom-client';


You should not read attributes in the constructor

tomalec · 2018-05-16T14:10:14Z

palindrom-error-catcher.html

+                this._bindCallbacks();
+            }
+            _bindToElement() {
+                this.target = this.getRootNode().querySelector(this.targetSelector);


I'dcheck in the document if there is none in the current root, as this element may be used in SD composition, there is no palindorm-client

tomalec · 2018-05-16T14:11:06Z

palindrom-error-catcher.html

+            _bindToElement() {
+                this.target = this.getRootNode().querySelector(this.targetSelector);
+            }
+            _bindCallbacks() {


I'd move it out of public API

tomalec · 2018-05-16T16:55:25Z

Tests fail locally in FF for me

also, could you rename d-b-n_smoke/simple.html to something more applicable, we all forgot about dom-bind-notifier luckily ;)

alshakero · 2018-05-16T16:57:03Z

Daaaamn! I had no idea d-b-n meant dom-bind-notifier. It was a huge mystery to me haha!

tomalec · 2018-05-16T17:03:28Z

Could you add CONTRIBUTING.md, LICENSE, issue_template and make README.md more elaborate as in https://github.com/Juicy/juicy-element-boilerplate (usage, attributes, etc.)

tomalec · 2018-05-16T17:04:29Z

palindrom-error-catcher.html

+                this.genericErrorPane.addEventListener('click', reload);
+                this.subscribed = true;
+            }
+            _unsubscribeToEvents() {


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 ;)

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.

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-classes

You 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.

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 :)

- Fix tests for Firefox - Remove functions from element API

…m/palindrom-error-catcher into initial-reviewable-code

alshakero · 2018-05-17T11:54:32Z

Should be ready

- Add contributing.md - Add Gruntfile for auto version bumping - Extend Readme.md - Add issue template

tomalec · 2018-05-17T18:46:48Z

Is it desired to change the behavior so it always reloads the page after few secs in case of the server crash? if so, then LGTM

tomalec

Checked the code and tested with Kitchensink

alshakero · 2018-05-18T08:35:40Z

Is it desired to change the behavior so it always reloads the page after few secs in case of the server crash? if so, then LGTM

Maybe I'm wrong, but isn't this what we do now?

tomalec · 2018-05-18T10:47:27Z

You're right. Then it's 100% fine to me.

Initial reviewable code

0c80a13

alshakero requested a review from tomalec May 16, 2018 11:40

tomalec reviewed May 16, 2018

View reviewed changes

Fix README typos and use Node 7 in Travis

2a67998

tomalec requested changes May 16, 2018

View reviewed changes

alshakero and others added 2 commits May 16, 2018 16:21

Address review comments from #1

5c60097

Add Sauce Labs keys

a213763

alshakero added 2 commits May 16, 2018 18:58

Move simple.html test outside

a469b63

Change paths in simple.html

866f690

tomalec reviewed May 16, 2018

View reviewed changes

alshakero added 3 commits May 17, 2018 13:05

Fixes

adbdb8e

- Fix tests for Firefox - Remove functions from element API

Merge branch 'initial-reviewable-code' of https://github.com/Palindro…

bca2e1b

…m/palindrom-error-catcher into initial-reviewable-code

Fix Edge browser name in wct.conf

8af681e

alshakero force-pushed the initial-reviewable-code branch from 5e93b39 to 8af681e Compare May 17, 2018 11:38

alshakero force-pushed the initial-reviewable-code branch 2 times, most recently from fa07c85 to a51a4c7 Compare May 17, 2018 15:33

Better paper work

5602c7d

- Add contributing.md - Add Gruntfile for auto version bumping - Extend Readme.md - Add issue template

alshakero force-pushed the initial-reviewable-code branch from a51a4c7 to 5602c7d Compare May 17, 2018 15:36

tomalec approved these changes May 17, 2018

View reviewed changes

alshakero merged commit 05b9dd1 into master May 21, 2018

alshakero deleted the initial-reviewable-code branch May 21, 2018 07:48

alshakero mentioned this pull request May 21, 2018

Remove connection bar and test for disconnection events Palindrom/palindrom-client#71

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial reviewable code #1

Initial reviewable code #1

alshakero commented May 16, 2018 •

edited

Loading

tomalec May 16, 2018

tomalec May 16, 2018

alshakero May 16, 2018

tomalec May 16, 2018

tomalec May 16, 2018

tomalec May 16, 2018

tomalec May 16, 2018

tomalec May 16, 2018

tomalec commented May 16, 2018

alshakero commented May 16, 2018

tomalec commented May 16, 2018

tomalec May 16, 2018

alshakero May 16, 2018 •

edited

Loading

tomalec May 17, 2018

tomalec May 17, 2018

alshakero commented May 17, 2018

tomalec commented May 17, 2018

tomalec left a comment

alshakero commented May 18, 2018

tomalec commented May 18, 2018


		> 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.


		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.

		Please check the code at `palindrom-error-catcher.html` file to see how events are handles.

Initial reviewable code #1

Initial reviewable code #1

Conversation

alshakero commented May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomalec commented May 16, 2018

alshakero commented May 16, 2018

tomalec commented May 16, 2018

Choose a reason for hiding this comment

alshakero May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alshakero commented May 17, 2018

tomalec commented May 17, 2018

tomalec left a comment

Choose a reason for hiding this comment

alshakero commented May 18, 2018

tomalec commented May 18, 2018

alshakero commented May 16, 2018 •

edited

Loading

alshakero May 16, 2018 •

edited

Loading