Some minor adjustments by yanshcherbakovatredspace · Pull Request #12 · theREDspace/HammerSpace

yanshcherbakovatredspace · 2018-01-08T19:14:58Z

Decoupling constructor so it only sets the crucial variables
Adding method connect so it can be overridden in case there's a different way that an application is connected to. An extra step will then be required:
rebound.connect()
Making some properties and methods protected so they can be used in sub-classes

lmckeen · 2018-01-08T19:22:33Z

+    }
+  }
+
+  //This is because a constructor should only be setting crucial variables.


can you please add proper JSDoc comments here

lmckeen · 2018-01-08T19:22:59Z

   */
  public dispatch: (name: ReboundEvent) => void = this._dispatch;
+
+  public connect: () => void = this._connect;


Can you add the JSDoc comment here

lmckeen · 2018-01-08T19:46:27Z

-  private _randId: string;
+  protected _randId: string;
+
+  protected _isHammerClient: boolean;


this is an unused variable... I would suggest adding the "no-unused-variable" property to tslint

👕 Should remove this one as ClientType provides a way to tell HammerSpace that it's not a HammerSpace instance on the other side.

Will remove.

lmckeen · 2018-01-08T19:48:39Z

+   * @protected
+   * @method connect
+   */
+  protected _connect() {


can you add tests for this please and thank you 😛

Since we changed the constructor parameter the test cases need to be adjusted, will adjust them along with the unit tests for _connect

lmckeen · 2018-01-08T20:59:53Z

rebound = new Hammer.Rebound({ id: 'appContainer', client: client, autoConnect: false });

make a small refactor so the user will now create a rebound object like above.
id and client are required and autoConnect is not required as it will have a default of true, this is to have less setup and to make it less confusing for the user. if autoConnect is false the user will have to make a call to rebound.connect();

Thanks 🙂

…d as modules, creating a union of types so addEvents accepts a single string and an array decoupling constructor to only set crucial variables and creating a connect method so it can be overridden.

lmckeen · 2018-04-26T17:30:36Z


-describe("Client:", () => {
+describe('Client:', () => {
  let client: any;


were you not able to remove this any too ?

If I do then the tests will fail because in typescript you should not be able to access them.

lmckeen · 2018-04-26T17:32:03Z

+    it('should only be able to add strings as new possible event', () => {
      let numOfKeysBefore = Object.keys(client._events).length;
-      let eventTypes = [1, [], {}, 1.2, undefined, null, true];
+      let eventTypes = [1, [], {}, 1.2, undefined, undefined, true];


this null was here as a test case for one of the different types of data that could be thrown at it

lmckeen · 2018-04-26T17:34:34Z

      client.addEvents('testevent');
-      client.on('testevent', function() {});
+      client.on('testevent', function() {
+        console.log('testevent');


is this so it does not yell at you for having an empty function? because this console.log is not required as it only checking to see if the property is actually there and not what the data is

yep just to remove a error

lmckeen · 2018-04-26T17:36:23Z

@@ -1,5 +1,5 @@
-describe("This test", function() {
-  it("will always pass", function() {
+describe('This test', function() {


could you add a comment to this so people know why this is here and that it should not be removed

yanshcherbakovatredspace self-assigned this Jan 8, 2018

lmckeen reviewed Jan 8, 2018

View reviewed changes

yanshcherbakovatredspace added 5 commits January 8, 2018 16:03

Making methods and variables protected so they can be extended if use…

87f26eb

…d as modules, creating a union of types so addEvents accepts a single string and an array decoupling constructor to only set crucial variables and creating a connect method so it can be overridden.

Adding JSDocs for the connect method 😊

cbfb73f

Adding ReboundConfig interface.

8632dee

Removing 👕 unneeded variable that was left not removed.

f982c74

Adjusting logic to use ReboundConfig.

c834840

yanshcherbakovatredspace force-pushed the feature/some-enhancements branch from b23b68b to c834840 Compare January 8, 2018 22:55

Minior formatting changes and spelling

f3a201a

SmithAF requested a review from Connor93 April 26, 2018 17:23

lmckeen reviewed Apr 26, 2018

View reviewed changes

spec changes

97dc8ff

SmithAF assigned SmithAF and unassigned yanshcherbakovatredspace Apr 27, 2018

lmckeen approved these changes Jun 8, 2018

View reviewed changes

Conversation

yanshcherbakovatredspace commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmckeen commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants