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

feat(features): initial work on making cell nav conditional #62

Merged
merged 7 commits into from
Jun 4, 2019

Conversation

JeffRiggle
Copy link

Initial work. This appears to be working to toggle between an active and inactive state. I think I will have to clean this up more but this is a first draft.


var row = rowCol.row,
col = rowCol.col;
function setupCellNav() {
Copy link

@zakeckert zakeckert May 29, 2019

Choose a reason for hiding this comment

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

I'm apparently really bad at following indentation, but you wrapped all of this stuff in setupCellNav, right?

Copy link
Author

Choose a reason for hiding this comment

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

Basically yeah. Most of this stuff has just been moved into this function so we can toggle the behavior

@JeffRiggle
Copy link
Author

Alright with these new revisions I think the feature is working as I would expect it to. Can everyone please take a read over this code before I start doing some refactoring. I think I need to move function definitions around a bit to make this code easier to read.

@@ -249,16 +249,19 @@
$rootScope.$emit.apply($rootScope, [eventId].concat(Array.prototype.slice.call(arguments)));
};

var cleanup = [];

Choose a reason for hiding this comment

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

This is basically the same thing as the destroySteps from above, right? You may want to consider unifying their naming convention

Copy link
Author

Choose a reason for hiding this comment

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

Yeah it's just scoped differently. I will start doing some real clean up an variable consistency type changes

}

destroySteps.forEach(function(destory) {
destory();

Choose a reason for hiding this comment

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

Typo in destory

evt.preventDefault();
}
// Refresh cell focus when a new row id added to the grid
dataChangeDereg = uiGridCtrl.grid.registerDataChangeCallback(function (grid) {

Choose a reason for hiding this comment

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

If everything else just adds to destroySteps, why not do that here too?

Copy link

@stormojm stormojm left a comment

Choose a reason for hiding this comment

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

Seems all resonable to me, but its hard to tell for sure as its so large...

@JeffRiggle JeffRiggle marked this pull request as ready for review May 31, 2019 19:07
@JeffRiggle JeffRiggle merged commit 845b6cd into master Jun 4, 2019
@JeffRiggle JeffRiggle deleted the conditional-cell-nav branch June 5, 2019 12:40
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