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

Jump to bottom

New option: sectionHeaders - display column headers within section labels #2218

Closed
wants to merge 1 commit into from
Closed

New option: sectionHeaders - display column headers within section labels #2218

wants to merge 1 commit into from

Conversation

russell-miller
Copy link

At Rentrak we had a need for an Angular grid that could display column headers in sections.

Our fork of ui-grid does so by swapping out the headerTemplate with a slightly different one that iterates over a new option sectionHeaders to eventually find values within columns array(s) - those being the names of real grid columns that it renders as it normally would.

Example:

$scope.gridOptions = {
  data: [],
  columnDefs: [
    { name: 'col1' },
    { name: 'col2' }
  ],
  sectionHeaders: [
    { displayName: 'Section', columns: [ 'col1', 'col2' ] }
  ]
};

Another need we had was to be able to take this as deep as we want per grid. So this is defined recursively in the new template. This means you can put sections within sections using the attribute subSections.

Example:

$scope.gridOptions = {
  data: [],
  columnDefs: [
    { name: 'col1' },
    { name: 'col2' },
    { name: 'col3' }
  ],
  sectionHeaders: [
    { displayName: 'Super Section', subSections: [
      { displayName: 'Section A', columns: [ 'col1', 'col2' ] },
      { displayName: 'Section B', columns: [ 'col3' ] }
    ]
  ]
};

Screenshot:
section-headers

Possible Concerns:

  • maybe this should have been a separate module instead of part of core? not sure how much more work would be entailed
  • there was a seemingly minor feature loss for any grids with section headers - the following style is not applied to the first column header in the template ng-style="$index === 0 && colContainer.columnStyle($index)"
  • no tests for the recursively defined directive - would be open to suggestions for how to add testing

Review on Reviewable

@c0bra
Copy link
Contributor

c0bra commented Dec 8, 2014

I know that this IS something that others have wanted, but I'm also wary of introducing it into the core. We're already seeing some feature bloat and there's probably some things currently in core that need to be split out.

If you want to release it as a plugin I'm happy to add a link to it from the site. Otherwise we can discuss the merits/drawbacks of including it in the core here.

Additionally, I'm not sure how it would comport with column re-ordering, column hiding/adding, etc.

@russell-miller
Copy link
Author

@c0bra thanks for the response, perhaps we could discuss a separate feature module solution?

@c0bra
Copy link
Contributor

c0bra commented Dec 11, 2014

Sure, that's a possibility. The place we're at is that the minified js is around 190k, about twice the size of angular itself. I'm afraid of continuing to just include feature-based stuff in the combined js, and in fact several if not all of the features, such as pinning/expandable grid/import&export/infinite-scroll, etc. should probably be moved to their own separate file.

@russell-miller
Copy link
Author

@c0bra bad news - found out last night column hiding doesn't work with this feature, though I'm trying to figure out why and fix it.

edit: I've got my sights on Grid.prototype.buildColumns and Grid.prototype.refresh (they seem to be called from Grid.prototype.columnRefreshCallback when I send gridApi.core.notifyDataChange and pass the constant dataChange.COLUMN per the tutorial for toggling column visibility) to see if there's something I need to do to refresh the custom headers... any tips would be greatly appreciated :)

edit: Update! Hiding and showing columns is now working with this feature. I was still repeating over sections/columns that were hidden, so it was a pretty easy fix.

@russell-miller russell-miller force-pushed the enh-section-headers branch 2 times, most recently from 0cccd3a to ff76a15 Compare December 16, 2014 17:26
@c0bra
Copy link
Contributor

c0bra commented Dec 22, 2014

Could you squash these commits into one so they're easier to review? You can force-push them to this branch to update this PR.

Thanks in advance.

@russell-miller
Enh(headers): add sectionHeaders grid option for enabling section hea…
37000c2 
…ders

    tasks:
     - add getSectionWidth method
     - add isSectionVisible method
@russell-miller
Copy link
Author

@c0bra sorry, I develop in a commit-often / keep-things-separated style... but don't worry I put my commits on a different branch 😀 -- and this branch is squashed down to just one.

@mportuga
Copy link
Member

@PaulL1 You marked this as waiting-response, but it seems that @russell-miller has already addressed all of @c0bra's concerns. It looks good to me, but I was wondering if you had any lingering concerns. Same goes for @c0bra.

@russell-miller
Copy link
Author

@mportuga I forgot I still had this request open... I ended up taking a different approach actually. There are conflicts with master now, and I hadn't fully tested that my changes didn't break other features of ui-grid. I would vote to just decline the request, though you may feel free to pick up where I left off.

cc: @PaulL1

@mportuga
Copy link
Member

@russell-miller Thank you for your reply. I am will simply decline it now as you've taken a different approach. I hadn't noticed the conflicts earlier, so thank you for pointing that out as well. I am new to the ui-grid team and I am trying to clean up all of these outstanding Pull requests.

@mportuga mportuga closed this Jul 26, 2016
@PaulL1
Copy link
Contributor

PaulL1 commented Jul 27, 2016

@mportuga: back then it was just a process to keep a track of why something wasn't progressing. I have no opinion on it today. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Looks Good To Me waiting-response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@russell-miller @c0bra @mportuga @PaulL1