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

DataViewModel Supports Multiple, Settable SortBy and GroupBy Fields #1715

Merged
merged 12 commits into from
Feb 26, 2020

Conversation

petradish
Copy link
Contributor

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.
  • [N/A] Reviewed and tested on Mobile, or determined not required.
  • [N/A] 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.

This PR addresses #1681
DataViewModel now pushes all store fields as columns but renders only the sortBy field as visible. This allows for multiple sorters and groupings as well as making them settable through GridModel's trampolined setter methods.

@petradish petradish linked an issue Feb 25, 2020 that may be closed by this pull request
@petradish petradish changed the title Data view set group by DataViewModel Supports Multiple, Settable SortBy and GroupBy Fields Feb 25, 2020
return field !== sortBy;
}).map(field => {
return {
colId: field,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I need colIds here, as field seems to be sufficient identification (at least in the toolbox examples)

@lbwexler
Copy link
Member

Petra and I working through this one.

@petradish petradish linked an issue Feb 26, 2020 that may be closed by this pull request
cmp/grid/Grid.js Outdated
@@ -201,6 +201,7 @@ class LocalModel {
},
popupParent: document.querySelector('body'),
suppressAggFuncInHeader: true,
suppressMakeColumnVisibleAfterUnGroup: true,
Copy link
Member

Choose a reason for hiding this comment

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

I thought a bit more about this -- let's just apply this now in DataView's agOptions, where we know we absolutely want this, and that will allow us to move forward with our DataView enhancements.

Think changing Grid default itself needs to be done with a bit more care, testing, and consideration of what we want for our grouping API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I moved it to DataView only and its localModel now accepts props and agOptions defined as PT to allow for overriding of the suppressMakeColumnVisibleAfterUngroup prop

…umnVisibleAfterUngroup is set to true

Removed property from Grid
@lbwexler
Copy link
Member

Discussed with Petra - she will enhance toolbox with better examples here, but looking forward to getting this in to v30 to more fully round-out the API changes to DataView/

@lbwexler lbwexler merged commit 421b17c into develop Feb 26, 2020
@lbwexler lbwexler deleted the dataViewSetGroupBy branch February 26, 2020 23:26
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.

DataViewModel groupBy should be settable
2 participants