-
-
Notifications
You must be signed in to change notification settings - Fork 29
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(core): add optional Top-Header for Drag Grouping & Header Grouping #1556
Conversation
- prior to this PR, we could not use Draggable Grouping & Header Grouping because both features were using the pre-header and it would have conflict, now to avoid this I am adding an extra Top-Header that will show over all header and pre-header. - NOTE: Draggable Grouping can now be displayed in either the new Top-Header OR the Pre-Header. However, the users who do want to use both features will need to explicitely enable both top/pre.
Run & review this pull request in StackBlitz Codeflow. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1556 +/- ##
========================================
+ Coverage 99.8% 99.8% +0.1%
========================================
Files 198 198
Lines 21691 21722 +31
Branches 7253 6996 -257
========================================
+ Hits 21630 21661 +31
- Misses 55 61 +6
+ Partials 6 0 -6 ☔ View full report in Codecov by Sentry. |
cc @zewa666 so I came up with a new Top-Header (as explained above), if you have a better name then please let me know. This new Top-Header is created on top of all the headers hence his name. Prior to this PR, the example above was not possible. Header Grouping (those are grouped header titles, e.g. "Period") will always use and require the pre-header to exist. | Top Header | Side note, how is it going with your PRs? I'm probably going to target a release by next weekend, so if you need more time then we could release future patches. I would prefer to include everything in 1 go, but there's really no pressure at all, I'm just planning ahead :) |
Aggrid calls them simply columngroups but I dunno if this fits this usecase. I do wonder though if grouping is supported for backendservices. my PRs are somewhat stuck atm as I'm getting my app ready for stage and prod these days. So I guess I'll get back to it next week along with the first user feedback. so far really the only feedback of a missing feature was the individual row heights along with cell content wrapping but I managed to dodge that bullet with customTooltips 😅 |
Header Grouping works as shown in Example 10, but Column Grouping is not supported for backend (well it's just partially supported and not what users are expecting since the way I've got Backend Services working is to always flush the DataView data every time you change page, and since Grouping is built by the DataView then you can only support Grouping for the data you currently have which is equal to Grouping 1 page and is not what users want). I had to write to over and over because many users want this feature but it's just too much work and I don't want to spend time on supporting this (I actually wouldn't even know where to start with this since like I said it's the DataView that supports Grouping with current data and we surely don't want to load the entire dataset because that goes against the Backend Service philosophy).
yup that's also another subject that was discussed so many times and is not quite possible since SlickGrid uses div and virtual scroll. It's actually doable with a lot of work to support dynamic row height, it was explained by Violet in this SO and I actually took some of that concept to create the Row Detail plugin (you can see her article Wooohoo! a responsive SlickGrid with dynamic row-heights, there's a link on the top of the article that you can click to see all her steps, there's a lot). Note that even if she's somehow showing dynamic row height, it has to follow a row height multiple which is what I followed in Row Detail (i.e. you tell the plugin how many rows you want to use) So I guess I'll push a new release by the end of this week and you can go back to your PRs whenever you have time. I want to clear up all the PRs I enqueued in each repos (7x in each). So I need to finish this PR and I guess that I'll stick with Top-Header name (I think it's fine, I certainly don't want to use "prepreheader" lol). I also have to look into a small bug that someone just opened this morning on Angular-Slickgrid Tree Data |
sure go ahead dont need to wait for me. I'd like to make sure to test out those changes properly and than finish the PRs missing parts like docs and tests |
TODOs
impactedColumns
#1557