Skip to content
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

[Time Conductor] Conductor pan, zoom, and time of interest #1287

Closed
wants to merge 188 commits into from

Conversation

akhenry
Copy link
Contributor

@akhenry akhenry commented Oct 24, 2016

Ok, I think this is good to go.There is minimal TOI support in old plots with known issues. Don't want to invest time in properly supporting old plots. I could also be persuaded to remove support from old plots, if we decide that no support is better than partial support.

Changes

Author Checklist

  1. Changes address original issue? Y
  2. Unit tests included and/or updated with changes? Y
  3. Command line build passes? Y
  4. Changes have been smoke-tested? Y

akhenry and others added 30 commits July 7, 2016 09:47
Fixes #993
In-progress; class renaming continued,
cleanups in markup file, in-page styles
ported to scss
Fixes #993
In-progress; Create menu refactoring and new
mini Create menu
Fixes #993
In-progress; mode menu names and
descriptors modified, markup cleaned up
Fixes #933
Resolve conflicts in
mode-menu.html, mode-selector.html,
time-conductor.html; apply tweaks, language, etc.
Fixes #933
Tweaks to language; tweak to class name in markup
Fixes #933
Conflicts:
	platform/features/conductor-v2/src/TimeConductorController.js
Fixes #933
Renamed main class; removed unused <style>
defs
Fixes #933
Color adjusts, mini super-menu size
and font tweaks, glyphs added to selector,
SVG style fixes in progress
Fixes #933
In-progress; fixed SVG text color
Fixes #933
In-progress: fixed SVG text color, field
styling for fixed vs. real-time, markup cleanup
Fixes #933
In-progress: color tweaks, bar sizing,
field widths
Fixes #933
In-progress: restructured markup to move
modeModel farther out; animated icon
Fixes #933
Fair amount of manual fixing in time-conductor.html
Fixes #933
Fair amount of manual fixing in time-conductor.html
Fixes #933
In-progress: color/size tweaks, fixes for espresso
theme
@akhenry
Copy link
Contributor Author

akhenry commented Nov 9, 2016

@larkin assigned this to you as you're most familiar with the TC now, so the delta should be easier to review. It's a lot to review, again, so if you want to hand this off to @VWoeltjen that's also fine.

@akhenry
Copy link
Contributor Author

akhenry commented Nov 9, 2016

  • removed the timeConductor service from the conductor bundle, and the conductorService from the compatibility bundle. Actually not sure what the latter was doing in there, it served no purpose. If any of the VISTA components are relying on either of these services (I'm thinking especially of the timeConductor one, they will need to get the conductor instance via other means. I'd suggest just passing in the openmct api object which is what I've done. An alternative is to define a new timeConductor service that just exposes openmct.conductor.

function ($) {

/**
* The mct-conductor-axis renders a horizontal axis with regular
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this comment is a leftover from copy/pasting.

var PADDING = 1;

/**
* The mct-conductor-axis renders a horizontal axis with regular
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outdated comment

* Set time of interest
* @param e The angular $event object
*/
ConductorTOIController.prototype.click = function (e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be clearer to name this function based on what it does rather than the event it is following?

}
};

ConductorTOIController.prototype.changeTimeOfInterest = function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no JSDoc, minimally should tag method as private.


}

ConductorTOIController.prototype.destroy = function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsdoc

Copy link
Contributor

@larkin larkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akhenry lots of missing jsdoc and some unimplemented tests, sending back to you to address.

var mockConductorViewService;

describe("The ConductorTOIController", function () {
mockConductor = jasmine.createSpyObj("conductor", [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incomplete tests for this controller.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, whoops!

this.conductorViewService.off('pan-stop', this.onPanStop);
};

TimeConductorController.prototype.changeBounds = function (bounds) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsdoc

@@ -137,23 +176,26 @@ define(
});
};

/**
* @private
*/
TimeConductorController.prototype.setFormFromDeltas = function (deltas) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing jsdoc

}
};

TimeConductorController.prototype.onZoomStop = function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsdoc

this.$scope.boundsModel.end = bounds.end;
};

TimeConductorController.prototype.onPanStop = function () {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jsdoc

expect(controller.$scope.boundsModel.end).not.toBe(testBounds.end);

// use registered CB instead
// controller.onPan(testBounds);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented code? remove?

//Calculate the previous raw end value (without delta)
oldEnd = oldEnd - this.dlts.end;
var bounds = this.calculateBoundsFromDeltas(deltas);
this.dlts = deltas;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason we can't just call this "deltas" instead of abbreviating it?

Copy link
Contributor Author

@akhenry akhenry Nov 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the deltas method on the prototype. I'll rename it something less rubbish though.

function ($) {

/**
* The mct-conductor-axis renders a horizontal axis with regular
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outdated jsdoc (copypaste?)


define(
["zepto"],
function ($) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like zepto is unused here.

$scope.$on('$destroy', this.destroy);
}

TimeOfInterestController.prototype.changeTimeOfInterest = function (toi) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing JSDoc all over

@larkin larkin added this to the Roddenberry milestone Nov 17, 2016
@larkin larkin removed their assignment Nov 17, 2016
@akhenry
Copy link
Contributor Author

akhenry commented Nov 22, 2016

@larkin addressed review comments, thanks.

@charlesh88 some glyph related conflicts. What's the best approach here?

@larkin
Copy link
Contributor

larkin commented Nov 25, 2016

I can resolve the icomoon conflicts-- resolved conflicts in the icomoon json file, change some ids (as two icons were created in different branches), uploaded to icomoon, generated new font files & json, and copied them back into project.

There is a small conflict with icon-refresh-- it was updated in master recently and also in this branch months ago.

Master:
screen shot 2016-11-25 at 2 45 50 pm

Pull Request:
screen shot 2016-11-25 at 2 46 16 pm

It looks like the refresh icon in master is not properly centered, but the refresh icon in this branch is. I believe the refresh icon in this branch is "more correct" than the version in master and will keep it. @charlesh88 can you confirm?

@larkin larkin assigned larkin and unassigned akhenry Nov 25, 2016
@larkin
Copy link
Contributor

larkin commented Nov 25, 2016

Squash-merged and pushing results in 79b4f9a.

Reviewer Checklist

  1. Changes appear to address issue? Y
  2. Appropriate unit tests included? Y
  3. Code style and in-line documentation are appropriate? Y
  4. Commit messages meet standards? Y

@larkin larkin closed this in 79b4f9a Nov 25, 2016
@larkin larkin deleted the open1193 branch November 25, 2016 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants