Migrating away from AngularJS #16

AmitMY · 2017-09-07T06:40:40Z

Removing the dependency in AngularJS, moving to custom elements in ES6 and SCSS, more tests, tests coverage, better files structure.

BLOG.md files is the work in progress blog post to release after merging. It contains all the information on what was done.

Fixes:
#2 - Adds a readyEvent Event emitter, whenever it redraws (new SVG)
#4 - Using an event emitter, can unsubscribe

Like everything, this needs many many many more tests, as the main provider is not fully covered.

…tOptions method

ramtob

Great job. I entered several questions, comments, and requests for small changes.

ramtob · 2017-09-10T08:03:03Z

BLOG.md

@@ -0,0 +1,49 @@
+# Revisiting Force-Horse: Making a Legacy Plugin Agnostic and Usable


A very nice blog post and plan. A small point: it seems an exaggeration to call force-horse a "legacy" plugin. It is not that old.. it is just that javascript is developing so fast. I probably wrote it just before ES6 kicked in.

Thanks!
It is legacy, just because AngularJS is legacy, see:

Legacy code refers to an application system source code type that is no longer supported

Yeah, you kind of mixed some ES6 in there, just a bit.

ramtob · 2017-09-10T08:11:09Z

BLOG.md

+While you can read all of the details on the library in Ram's [blog post](http://webiks.com/force-horse-force-layout-component/), the gist of it is having a library that adapts to many types of data sets, and is easily modifiable for the users, having many modes for nodes, D3.js forces, multiple edges between nodes, supporting any size of data set, and easily allowing customization of the base SVG's design.
+
+## So... What's New?
+Having the library written using AngularJS, forces the user to use a deprecated framework. Either the user thinks they can just include AngularJS in the bundle, increasing the bundle, or trying to implement the component for their used framework.


A small comment: AngularJS is, of course, not the front version of Angular, but it is not deprecated. It will be with us perhaps for a long time still..

ramtob · 2017-09-11T09:12:28Z

README.md

@@ -68,38 +100,37 @@ The config parameters are:
 }
 ```

-The forceHorse project contains a complete **demo application**. The demo application is also available on the [plunker](http://embed.plnkr.co/SYmehtaAnQVyMpLJJY2B/?show=preview) site.
+The forceHorse project contains a complete **demo application**. The demo application is also available on the [plunker](http://embed.plnkr.co/SYmehtaAnQVyMpLJJY2B/?show=preview) site.


The demo on plunker is probably out of date

Hahaha probably.
I'll make a new one on the updated code base, as the package is on NPM already (had to test it for the demo)

ramtob · 2017-09-12T11:19:54Z

src/components/force-horse/force-horse.js

@@ -0,0 +1,63 @@
+import {ForceHorseProvider} from '../../providers/force-horse';


It seems that you have three files with the same name force-horse.js (in different folders). This does not seem to me a good practice. Files tend to travel between folders. Better to have a unique name for each file, like force-horse-component.js, force-horse-helper.js, etc.

Reasonable. Done

ramtob · 2017-09-12T11:26:56Z

src/providers/force-horse.js

+/**
+ * Produces a class-instance for each instance of ForceHorse on a page
+ */
+export class ForceHorseProvider {


Why "provider" in the class name? Why not "factory" as it was before?

Because it does not instantiate a service, and just provides logic for the component. Can change if you find this awful

ramtob · 2017-09-12T11:31:27Z

webpack.config.js

+        use: extractSASS.extract({ // Instance 2
+          use: [
+            'css-loader',
+            'postcss-loader',


Why both postcss loader and sass loader?

because I use scss, and because I want autoprefixer for -webkit-mask.., so sass-loader for scss, and postcss for autoprefixer

ramtob · 2017-09-12T11:42:48Z

.travis.yml

@@ -0,0 +1,23 @@
+language: node_js


I am not so familiar with Travis. What does this file do?

Every commit, travis will automatically try to tests. and report back on github what is the status
If there is a new tag, it will deploy it to a github release, and to an NPM release, hopefully

See below, it shows a PR status

ramtob · 2017-09-12T11:49:03Z

BLOG.md

+While you can read all of the details on the library in Ram's [blog post](http://webiks.com/force-horse-force-layout-component/), the gist of it is having a library that adapts to many types of data, is easily modifiable for developers, supports large data sets, and easily allowing customization of the base SVG's design.
+
+## So... What's New?
+Having the library written using AngularJS, forces the user to use a deprecated framework. Either the user thinks they can just include AngularJS in the bundle, increasing the bundle, or trying to implement the component for their used framework.


AngularJS is not, strictly speaking, deprecated. It is still supported, and widely used.

You are right. Changed from "deprecated" to "legacy".

ramtob · 2017-09-12T11:54:33Z

BLOG.md

+Therefore, we decided to remove the dependency for AngularJS, making the library framework agnostic.
+
+##### Agnostification
+We migrated AngularJS out of the library, moving to a standard browser feature - [Custom Elements](https://developer.mozilla.org/en-US/docs/Web/Web_Components/Custom_Elements), supported by most modern browsers, or being worked on at this time (In case it isn't supported, there is a tiny [browser polyfill](https://github.com/webcomponents/webcomponentsjs) - see [browsers support](https://caniuse.com/#search=custom%20elements%20v0))


Actually you seem to have used here Custom Elements V1, which is not yet (as of 09.17) supported by most modern browsers.

The link to caniuse.com needs to be fixed (V1)

Can we ship the polyfill inside our package? (and, say, check whether the browser supports custom elements V1, and if not, automatically load the polyfill)

Done

Done

We can, we shan't imo. It is super simple to add, I can add the code to the README, it is like:

npm i @webcomponents/custom-elements

Add <script src="custom-elements.min.js"></script> or import from '@webcomponents/custom-elements/custom-elements.min.js'

The reason I think we shan't is that if you use Angular or React or whatever, it is just bloat.

ramtob · 2017-09-14T08:46:59Z

It looks fine, I'm just still a bit uncomfortable with the word "provider"... Did you see the word "provider" used in this way anywhere else? I know the word is used in AngularJS, but it has a different meaning there (a kind of service generator). What do you think about "viewer" instead? Because what you called the force horse "component" is actually a container, wrapping the graph viewer (what you called "provider") and the buttons bars.

AmitMY · 2017-09-14T08:56:42Z

@ramtob Ok, done.
I also moved data and config to be attributes instead of options, as it makes more sense in an operative stand point, and added Angular integration in the README

ramtob · 2017-09-14T09:10:28Z

Good. Just one more little request: instead of forceHorse.instance and this.instance, call it forceHorce.viewer and this.viewer. This will be more informative and readable (this.instance is actually a kind of duplication, since "this" is already an indication for an instance)

AmitMY · 2017-09-14T09:14:15Z

@ramtob Fair point, changed that.
Plunker/Demo apps will be up after your PR approval

ramtob · 2017-09-14T09:18:47Z

I approved the PR. Nice work

AmitMY · 2017-09-14T09:20:28Z

Thanks!

Amit Moryossef and others added 5 commits September 4, 2017 14:45

chore(): moving to the main repo

274d4b4

tests(): add tests for event emitter and buttons

591d4b4

tests(): add tests for buttons and main component

127fc85

tests(): add tests for component and provider

5885b06

chore(tests): add tests coverage

0c1de8f

AmitMY requested a review from ramtob September 7, 2017 06:40

AmitMY added 13 commits September 7, 2017 10:10

docs(blog): add a blog

6825442

fix(coverage): tests coverage works

e643de7

fix(buttons): add mask no repeat

2984d1f

refactor(): remove the change for options, add a not-DOM intensive se…

af85e91

…tOptions method

feat(travis): add travis for release and npm

4e84769

fix(travis): use ci script

b213039

docs(blog): tiny edit

7a18582

docs(blog): fix footer

17c3a86

docs(README): update to current usage

01e2231

docs(README): fix formatting

e8f7e43

docs(README): fix formatting

29d0211

docs(): general info

05bd4e3

fix(component): fix attribute binding

60fde33

ramtob requested changes Sep 12, 2017

View reviewed changes

AmitMY added 5 commits September 12, 2017 15:22

docs(BLOG): AngularJS is not deprecated

d26020c

docs(BLOG): use custom elements v1

415a043

refactor(names): rename files

d10fb47

refactor(): change options to config and data

63fb91c

chore(): bump version

0f3b239

refactor(): rename force-horse-viewer

0aaa086

fix(): moved to viewer

2f71e07

refactor(instance): change to viewer

3381b76

ramtob merged commit 73e7de0 into master Sep 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating away from AngularJS #16

Migrating away from AngularJS #16

AmitMY commented Sep 7, 2017 •

edited

Loading

ramtob left a comment

ramtob Sep 10, 2017

AmitMY Sep 12, 2017

ramtob Sep 10, 2017

ramtob Sep 11, 2017

AmitMY Sep 12, 2017

ramtob Sep 12, 2017

AmitMY Sep 12, 2017

ramtob Sep 12, 2017

AmitMY Sep 12, 2017

ramtob Sep 12, 2017

AmitMY Sep 12, 2017

ramtob Sep 12, 2017

AmitMY Sep 12, 2017

AmitMY Sep 12, 2017

ramtob Sep 12, 2017

AmitMY Sep 12, 2017

ramtob Sep 12, 2017

AmitMY Sep 12, 2017

ramtob commented Sep 14, 2017

AmitMY commented Sep 14, 2017

ramtob commented Sep 14, 2017

AmitMY commented Sep 14, 2017

ramtob commented Sep 14, 2017

AmitMY commented Sep 14, 2017

		@@ -0,0 +1,49 @@
		# Revisiting Force-Horse: Making a Legacy Plugin Agnostic and Usable

		@@ -0,0 +1,63 @@
		import {ForceHorseProvider} from '../../providers/force-horse';

Migrating away from AngularJS #16

Migrating away from AngularJS #16

Conversation

AmitMY commented Sep 7, 2017 • edited Loading

ramtob left a 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

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

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

ramtob commented Sep 14, 2017

AmitMY commented Sep 14, 2017

ramtob commented Sep 14, 2017

AmitMY commented Sep 14, 2017

ramtob commented Sep 14, 2017

AmitMY commented Sep 14, 2017

AmitMY commented Sep 7, 2017 •

edited

Loading