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

fix(gridMenu): add missing "headerColumnValueExtractor" to Grid Menu & fix #490 #491

Merged
merged 5 commits into from
May 18, 2020
Merged

fix(gridMenu): add missing "headerColumnValueExtractor" to Grid Menu & fix #490 #491

merged 5 commits into from
May 18, 2020

Conversation

ghiscoding-SE
Copy link
Contributor

@ghiscoding-SE ghiscoding-SE commented May 14, 2020

When I created the Grid Menu control, I forgot to include the headerColumnValueExtractor function that the Column Picker already has, this is really helpful when having a grid with Column Header Title Grouping as can be seen below

@6pac in case you're wondering, @ghiscoding-SE is my work GitHub account, I work for Schneider Electric, hence the SE ;)

  • current Cypress E2E tests should be enough to cover this PR
  • also tested manually with my local setup

BEFORE

image

AFTER

image

- The "headerColumnValueExtractor" is especially useful when have Header Title Column Grouping if we want to include the title as part of the column picker of the Grid Menu
@ghiscoding ghiscoding changed the title fix(gridMenu): add missing "headerColumnValueExtractor" to Grid Menu WIP- fix(gridMenu): add missing "headerColumnValueExtractor" to Grid Menu May 14, 2020
@ghiscoding ghiscoding changed the title WIP- fix(gridMenu): add missing "headerColumnValueExtractor" to Grid Menu fix(gridMenu): add missing "headerColumnValueExtractor" to Grid Menu May 14, 2020
@ghiscoding ghiscoding changed the title fix(gridMenu): add missing "headerColumnValueExtractor" to Grid Menu fix(gridMenu): add missing "headerColumnValueExtractor" to Grid Menu & fix #490 May 14, 2020
@ghiscoding
Copy link
Collaborator

@6pac
Hey mate, sorry about all these PRs, I was trying to fix as many issues as possible before asking (begging) for another version release. I'm expecting major versions in my libs in coming and I would really like to have all these fixes flowing to my libs as well. Thanks again my friend.

@6pac
Copy link
Owner

6pac commented May 18, 2020

no probs, if they look uncontroversial and well thought out, which they usually are, I'm happy to just put them through.

Also happy to do a release whenever. As I've probably said, I have a habit of waiting a few days after a PR is ready because often a few small issues pop up, but if you need them fast, let me know.

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 18, 2020

Yup, as you can imagine I like to tests my code as much as I can ;)
I now have 3 repos (and frameworks) using your fork, that's a lot to test with hehe

I'm expecting to release new version of my libs around mid next week, so if you could have a new release by then, that would be great, and so it doesn't have to be today if you wish to wait and take your time it's all good.

@ghiscoding ghiscoding merged commit eec27b7 into 6pac:master May 18, 2020
@ghiscoding
Copy link
Collaborator

@6pac I'll merge this PR and run some more tests on my side with all merged code of recent commits and then report back afterward.

@ghiscoding ghiscoding deleted the bugfix/grid-menu-column-value-extractor branch May 18, 2020 15:46
@ghiscoding
Copy link
Collaborator

ghiscoding commented May 18, 2020

@6pac good thing I tried, I found a small problem when having 2 grids in 1 page. I'll look at creating another PR to fix this... Actually scratch that, the problem was always present (I just never noticed it earlier) and is probably in my lib itself not in SlickGrid, it seems like a Singleton issue.

EDIT

I fixed the problem, and explained it in there, in the next PR #500 ... oh wow we just reached 500, how lucky me lol

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.

3 participants