refactor(YawPitchControl): add unit test and refactor src #42

happyhj · 2017-10-30T09:04:26Z

Issue

#35

Details

add unit test and refactor YawPitchControl.js for better testability.
also, fixed typo at MoveKeyInput.spec.js

also, fixed typo at MoveKeyInput.spec.js ref #35

coveralls · 2017-10-30T09:06:42Z

Coverage increased (+0.5%) to 37.278% when pulling d93989b on YawPitchControl#35 into a396865 on master.

jongmoon · 2017-10-31T01:35:04Z

src/YawPitchControl/YawPitchControl.js

-				if (evt.delta.fov !== 0) {
-					this._setPanScale(evt.pos.fov);
-					this._updateControlScale();
-				}
 				this._triggerChange();


I agree removing code because it is process by change event. but how about change event name? current is "change" but... I think it would be better to notice it's release event.

Whether or not to provide a release event will be discussed in a separate issue.

Issue opened
#43

jongmoon · 2017-10-31T01:51:29Z

src/YawPitchControl/YawPitchControl.js

 		this._applyOptions(changedKeyList, beforeOptions);
 		return this;
 	}

 	_option(key, value) {
-		if (arguments.length >= 2) {


As you know it makes available following.

this.option("key", "value")

And I think it is general pattern (interface) for setting value. If it is not unreachable it would be better to add some test case.

Actually, in '_option' method, it's not using key, value parameter anymore.
It supports three API patterns below.

this._option("key"); // Getter this._option(); // Global Getter this._option({ "key": value }); // Setter

The value portion of the argument is expected to be deleted.

jongmoon · 2017-10-31T02:02:53Z

src/YawPitchControl/YawPitchControl.js

 	}

 	_applyOptions(keys, prevOptions) {
 		// If one of below is changed, call updateControlScale()
 		if (keys.some(key =>
-				key === "showPole" || key === "fov" || key === "aspectRatio")) {
-			this._setYawPitchRange(prevOptions);


It's good to change code to exit recursive. but It is missing to change yaw/pitch range if fov or aspect ratio is changed.

I understand what you mean. I will fix that.

I changed _getValidYawRange, _getValidPitchRange methods to use new fov and aspectRatio parameter.

jongmoon · 2017-10-31T02:12:49Z

src/YawPitchControl/YawPitchControl.js

 		const horizontalFov = fov * ratio;
+		const isValid = !(newYawRange[1] - newYawRange[0] < horizontalFov);


Is it right? Isn't your intend following?

// ! is removed const isValid = (newYawRange[1] - newYawRange[0] < horizontalFov);

Although it's your intend I think It would be better to not put ! like below

const isValid = (newYawRange[1] - newYawRange[0] >= horizontalFov);

Thanks. I will fix that.

jongmoon · 2017-10-31T02:13:12Z

src/YawPitchControl/YawPitchControl.js

-			this.option("pitchRange", prevPitchRange);
+	_getValidPitchRange(newPitchRange) {
+		const fov = this.axes.get().fov;
+		const isValid = !(newPitchRange[1] - newPitchRange[0] < fov);


Same as above.

Thanks. I will fix that, too

jongmoon · 2017-10-31T02:19:38Z

src/YawPitchControl/YawPitchControl.js

+		if (isValid) {
+			return newPitchRange;
+		} else {
+			return this.options.pitchRange || [-PITCH_RANGE_HALF, PITCH_RANGE_HALF];


How about default range as fov ranges instead of DEFAULT?

jongmoon · 2017-10-31T02:20:33Z

test/unit/YawPitchControl/YawPitchControl.spec.js

+				// Then
+				const appliedOption = this.inst.option();
+				expect(appliedOption.yawRange).to.deep.equal([-180, 180]);
+				expect(appliedOption.pitchRange).to.deep.equal([-90, 90]);


It's related with code that I commented above.

You can ignore this comment.

jongmoon · 2017-10-31T02:24:21Z

test/unit/YawPitchControl/YawPitchControl.spec.js

@@ -203,6 +221,23 @@ describe("YawPitchControl", function() {
 				done();
 			}
 		});
+
+		it("should lookAt trigger change event once when enable called multiple times.", () => {
+			// Given


Is this case for checking axes not connected PanInput multiple times?

Yes! Exactly.

OK Thank you!

jongmoon · 2017-10-31T02:25:18Z

test/unit/YawPitchControl/YawPitchControl.spec.js

+				inst.option("yawRange", [-1, 1]);
+
+				// Then
+				console.log(inst.option("yawRange"));


No expect code.

I will add assertion.

jongmoon · 2017-10-31T02:25:30Z

test/unit/YawPitchControl/YawPitchControl.spec.js

+				inst.option("pitchRange", [-1, 1]);
+
+				// Then
+				console.log(inst.option("pitchRange"));


No expect code.

I will add assertion.

coveralls · 2017-10-31T11:32:01Z

Coverage increased (+0.7%) to 37.456% when pulling 21fbe0f on YawPitchControl#35 into a396865 on master.

jongmoon · 2017-11-01T02:09:04Z

src/YawPitchControl/YawPitchControl.js

@@ -168,25 +170,28 @@ const YawPitchControl = class YawPitchControl extends Component {

 		// adjustOption
 		if (changedKeyList.some(key => key === "yawRange")) {


@happyhj how about moving this call to _applyOption. and I have other things to talk . let's take meeting.

jongmoon

LGTM

coveralls · 2017-11-01T03:38:02Z

Coverage increased (+0.7%) to 37.439% when pulling cb08f66 on YawPitchControl#35 into a396865 on master.

refactor(YawPitchControl): add unit test and refactor src

d93989b

also, fixed typo at MoveKeyInput.spec.js ref #35

jongmoon self-requested a review October 31, 2017 01:31

jongmoon suggested changes Oct 31, 2017

View reviewed changes

happyhj mentioned this pull request Oct 31, 2017

Providing 'release' event #43

Closed

fix(YawPitchControl): apply review

21fbe0f

jongmoon reviewed Nov 1, 2017

View reviewed changes

fix(YawPitchControl): apply review

cb08f66

jongmoon approved these changes Nov 1, 2017

View reviewed changes

happyhj merged commit 98f6af5 into master Nov 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(YawPitchControl): add unit test and refactor src #42

refactor(YawPitchControl): add unit test and refactor src #42

happyhj commented Oct 30, 2017

coveralls commented Oct 30, 2017

jongmoon Oct 31, 2017

happyhj Oct 31, 2017 •

edited

Loading

jongmoon Oct 31, 2017

happyhj Oct 31, 2017 •

edited

Loading

jongmoon Oct 31, 2017

happyhj Oct 31, 2017 •

edited

Loading

jongmoon Oct 31, 2017

happyhj Oct 31, 2017

jongmoon Oct 31, 2017

happyhj Oct 31, 2017

jongmoon Oct 31, 2017

jongmoon Oct 31, 2017

jongmoon Nov 1, 2017

jongmoon Oct 31, 2017

happyhj Oct 31, 2017

jongmoon Nov 1, 2017

jongmoon Oct 31, 2017

happyhj Oct 31, 2017

jongmoon Oct 31, 2017

happyhj Oct 31, 2017

coveralls commented Oct 31, 2017

jongmoon Nov 1, 2017

jongmoon left a comment

coveralls commented Nov 1, 2017

		const horizontalFov = fov * ratio;
		const isValid = !(newYawRange[1] - newYawRange[0] < horizontalFov);

		@@ -168,25 +170,28 @@ const YawPitchControl = class YawPitchControl extends Component {

		// adjustOption
		if (changedKeyList.some(key => key === "yawRange")) {

refactor(YawPitchControl): add unit test and refactor src #42

refactor(YawPitchControl): add unit test and refactor src #42

Conversation

happyhj commented Oct 30, 2017

Issue

Details

coveralls commented Oct 30, 2017

Choose a reason for hiding this comment

happyhj Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

happyhj Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

happyhj Oct 31, 2017 • 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

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

coveralls commented Oct 31, 2017

Choose a reason for hiding this comment

jongmoon left a comment

Choose a reason for hiding this comment

coveralls commented Nov 1, 2017

happyhj Oct 31, 2017 •

edited

Loading

happyhj Oct 31, 2017 •

edited

Loading

happyhj Oct 31, 2017 •

edited

Loading