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

feat(core): add optional Top-Header for Drag Grouping & Header Grouping #1556

Merged
merged 6 commits into from
Jun 5, 2024

Conversation

ghiscoding
Copy link
Owner

@ghiscoding ghiscoding commented Jun 4, 2024

  • 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 headers.
  • I was getting a little out of ideas to get a name (since "pre" is already taken), so I decided to go with Top-Header which is completely on the top and even before the pre-header.
  • Another note is that this new Top-Header is not following the frozen columns (like header & pre-header do) and is rather always the same width as the grid. I purposely did not want this top-header to follow the grid frozen columns because I wanted a simpler header and also because I expect this to be mostly (and only) be used by the Draggable Grouping.

TODOs

  • need more unit tests
  • need more Cypress E2E tests (Example 3)
  • Example 18 is now broken (not currently able to show Draggable Grouping without Header Grouping)
  • throws an error when following these 2 steps (1. drag a column to group, 2. freeze a column)
  • see if we need docs update

brave_exnFdfLkAn

- 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.
Copy link

stackblitz bot commented Jun 4, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

codecov bot commented Jun 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.8%. Comparing base (c9fde21) to head (9d123d6).
Report is 3 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@ghiscoding ghiscoding changed the title feat(core): add optional Top-Header (over pre-header) feat(core): add optional Top-Header for Drag Grouping & Header Grouping Jun 4, 2024
@ghiscoding
Copy link
Owner Author

ghiscoding commented Jun 4, 2024

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 |
| Pre-Header |
| Headers |

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 :)

@zewa666
Copy link
Contributor

zewa666 commented Jun 4, 2024

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 😅

@ghiscoding
Copy link
Owner Author

ghiscoding commented Jun 4, 2024

Aggrid calls them simply columngroups but I dunno if this fits this usecase. I do wonder though if grouping is supported for backendservices.

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).

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 😅

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

@zewa666
Copy link
Contributor

zewa666 commented Jun 4, 2024

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

@ghiscoding ghiscoding merged commit 7d4a769 into master Jun 5, 2024
9 checks passed
@ghiscoding ghiscoding deleted the feat/top-header branch June 5, 2024 03:52
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.

None yet

2 participants