Skip to content

Conversation

cnrudd
Copy link
Member

@cnrudd cnrudd commented Aug 13, 2025

I found I need this feature on a tree grid where counting the grouping rows confuses things.

If we wanted to go further, seems like the store could also support some more getters, and then the logic inside GridCountLabel would be simplified.

get children(): StoreRecord[] {
     return this._filtered.filter(it => !it.children.length);
}

get allChildren(): StoreRecord[] {
     return this._current.filter(it => !it.children.length);
}

get childrenCount(): number {
      return this.children.length;
}

get allChildrenCount(): number {
      return this.allChildren.length;
}

Also, if this feature has legs, I would apply the same changes to StoreCountLabel.

Hoist P/R Checklist

Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.

  • Caught up with develop branch as of last change.
  • Added CHANGELOG entry, or determined not required.
  • Reviewed for breaking changes, added breaking-change label + CHANGELOG if so.
  • Updated doc comments / prop-types, or determined not required.
  • Reviewed and tested on Mobile, or determined not required.
  • Created Toolbox branch / PR, or determined not required.

If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.

Pull request reviewers: when merging this P/R, please consider using a squash commit to
collapse multiple intermediate commits into a single commit representing the overall feature
change. This helps keep the commit log clean and easy to scan across releases. PRs containing a
single commit should be rebased when possible.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new excludeParents prop to the GridCountLabel component to exclude parent records from the record count calculation. This feature is useful for tree grids where counting grouping/parent rows can be confusing.

  • Added excludeParents boolean prop to GridCountLabelProps interface
  • Modified count calculation logic to filter out parent records when both includeChildren and excludeParents are true
  • Updated CHANGELOG.md with the new feature

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
cmp/grid/helpers/GridCountLabel.ts Added excludeParents prop and implemented filtering logic to exclude parent records from count
CHANGELOG.md Added changelog entry documenting the new excludeParents property

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

unitLabel = count === 1 ? singularize(unit) : pluralize(unit);
let count = includeChildren ? store.count : store.rootCount;
if (excludeParents && includeChildren) {
count = store.records.filter(it => !it.children.length).length;
Copy link
Preview

Copilot AI Aug 13, 2025

Choose a reason for hiding this comment

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

This filtering operation on store.records is performed on every render when excludeParents and includeChildren are both true. Consider caching this calculation or using a memoized getter from the store to avoid repeated array filtering operations.

Copilot uses AI. Check for mistakes.

@amcclain
Copy link
Member

amcclain commented Aug 14, 2025

Hi - this is a familiar need for us, although it's been a while since we last discussed it. It's a tricky one.

See https://xhio.slack.com/archives/C4MF830H5/p1540589092038300 and https://xhio.slack.com/archives/C4MF830H5/p1701803743851219

A few things we should still consider:

  • This proposal defines a "parent" as "a record with non-zero amount of children". OK for parent rows where all children have been filtered out to "become" children? Maybe that's desired, or just doesn't matter?
    • Should we add StoreRecord.isParent getter if we're committing to this definition?
  • Copilot flags fact that we are allocating an array and filtering across all records to get this count, vs other cases which use pre-computed counts on RecordSet. Is that a practical concern?
  • It's only 8:30am and includeChildren + excludeParents making my head hurt a bit :) New param true ignored if old param false, should we just return nothing instead?
  • What about StoreCountLabel?

@cnrudd
Copy link
Member Author

cnrudd commented Aug 14, 2025

This proposal defines a "parent" as "a record with non-zero amount of children". OK for parent rows where all children have been filtered out to "become" children? Maybe that's desired, or just doesn't matter?

Doesn't seem to matter, is desired. See demo toolbox branch: gridCountLabelExcludeParents

Copilot flags fact that we are allocating an array and filtering across all records to get this count, vs other cases which use pre-computed counts on RecordSet. Is that a practical concern?

I am testing on a grid with 16k rows and I don't see any issues. Maybe on very large grids this would be a reason not to pre-compute children, allChidren in store, as suggested above, and just keep this logic on this component.

It's only 8:30am and includeChildren + excludeParents making my head hurt a bit :) New param true ignored if old param false, should we just return nothing instead?

Certainly could do that. Could also warn in console to alert developer that that configuration doesn't make sense.

What about StoreCountLabel?

I mentioned in description at top that whatever was done to GridCountLabel could be applied to StoreCountLabel.

@cnrudd
Copy link
Member Author

cnrudd commented Aug 14, 2025

I just tested on our toolbox companies grid, which is not a tree grid, but a grid with grouping. The problem of excess row counts (or counting groups and children when I only want children counted) doesn't happen there because the grouping is done at the agGrid level, not at the store level, so there are no parent/child relationships in the store. Every record is a root record.

So, just to help the discussion and limit the scope of problem, it only applies to when gridModel.treeMode = true.
This is something we could add to the docs for includeChildren right now, only applies to tree grids, not grids with groupBy, or, on a grid that uses groupBy, be aware that the count will not include grouping rows.

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.

2 participants