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

user-configurable expandableRowHeaderColDef #2396

Closed
wants to merge 4 commits into from

Conversation

@jadelane
Copy link

jadelane commented Dec 18, 2014

Related to issue #1990.

This allows the user to configure the expandableRowHeaderColDef by adding a $scope.gridOptions.expandableRowHeaderColDef to js files.

Review on Reviewable

@c0bra c0bra added the unknown label Dec 18, 2014
@PaulL1
Copy link
Contributor

PaulL1 commented Dec 19, 2014

I'm OK with this in concept, but two requests:

  1. Could we add ngdoc for the option? The format is pretty straightforward, for example starting at line 27 in the file. We'd also typically do the defaulting/initialisation in the initializeGrid method, rather than inline at the point you're using it (take a look at the top of the file to see what I mean)
  2. Could we put an example of this into the tutorial, or at least mention it in the tutorial? The tutorial files are in misc/tutorial/*.ngdoc

@jpuri, I'd also be interested in any comment you may have on this.

},
userInjectedExpandableRowHeaderColDef = $scope.gridOptions.expandableRowHeaderColDef || {},
finalExpandableRowHeaderColDef = angular.extend({}, defaultExpandableRowHeaderColDef, userInjectedExpandableRowHeaderColDef);
uiGridCtrl.grid.addRowHeaderColumn(finalExpandableRowHeaderColDef);

This comment has been minimized.

Copy link
@jpuri

jpuri Dec 20, 2014

Contributor

Please use var declaration for variables you are declaring. I could not find one for userInjectedExpandableRowHeaderColDef and finalExpandableRowHeaderColDef. Also code styling in the if block needs to be corrected.

@jpuri
Copy link
Contributor

jpuri commented Dec 20, 2014

Hey @PaulL1 PR looks fine to me. It will give users ability to customize row headers for expandable rows. I have added suggestions about the code changes @jadelane please check those.

@PaulL1 PaulL1 added this to the 3.0 milestone Dec 20, 2014
jadelane added 3 commits Dec 22, 2014
@jadelane
Copy link
Author

jadelane commented Dec 29, 2014

@PaulL1, Pull Request is ready for review again

@c0bra
Copy link
Member

c0bra commented Feb 18, 2015

@jadelane if you're able, it would be really good to rebase these commits into one single commit.

I would do it on my end but you'd be lost as the author and I wouldn't want you lose your "contributorship" :)

@PaulL1 PaulL1 modified the milestones: 3.1, 3.0 Apr 18, 2015
@JLLeitschuh
Copy link
Contributor

JLLeitschuh commented Jul 15, 2015

@c0bra You could allow them to keep authorship: http://stackoverflow.com/a/3308780/3708426

@JayHuang
Copy link

JayHuang commented Oct 22, 2015

This PR seems to cause the Expand All button/icon to disappear and show "Expandable Buttons" in its place.

@mportuga
Copy link
Member

mportuga commented Sep 20, 2016

@jadelane I am closing this PR for now since it has conflicts and @JayHuang pointed to a possible issue with it. Feel free to open a new one if you still wish to contribute.

@mportuga mportuga closed this Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.