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 #6409 - Datatable: ColumnGroup refactoring #9872

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

Rapster
Copy link
Member

@Rapster Rapster commented Mar 5, 2023

Fix #6409
Fix #9854

@Rapster Rapster marked this pull request as draft March 5, 2023 23:19
@tandraschko
Copy link
Member

before you work on a PR which refactors such a important feature, we should discuss what you refactor and if we are all okay with it; especially PrimeTek and if we can also be backward compatible

@Rapster Rapster changed the title Fix #3409 - Datatable: ColumnGroup refactoring Fix #6409 - Datatable: ColumnGroup refactoring Mar 6, 2023
@Rapster
Copy link
Member Author

Rapster commented Mar 6, 2023

PrimeTek has already been pinged regarding this, and no answer. Community is aware of it, and also I'm trying to make it backward compatible as much as I can. If you have any feedbacks, please feel free to share in the original issue

@Rapster Rapster linked an issue Mar 6, 2023 that may be closed by this pull request
4 tasks
@tandraschko tandraschko added this to the 14.0.0 milestone May 10, 2023
@melloware melloware added the enhancement Additional functionality to current component label May 26, 2023
@melloware melloware added the breaking-change Change is not backwards compatible label Jul 19, 2023
@nimo23
Copy link
Contributor

nimo23 commented Aug 15, 2023

@Rapster I created #9572 and I wanted to delve into the DataTable-component model to solve this problem. However, I see that you have already made many changes here in this PR, also in relation to #9572 (e.g. you did a lot of improvements in relation to frozenColumn, etc.), so it is not appropriate for me to delve into the "still old" code of the DataTable-component at this time.

So it would be cool if you maybe noticed how you can solve #9572 alongside with the code you refactored - maybe now it's much easier (for you) to solve #9572 with your refactored code.

You already have a deep understanding of the DataTable-component that I don't have. With the many code improvements in this PR, it might also occur to you to fix #9572 at the same time.

Perhaps #9572 is now much easier to fix than with the current "old code" of the DataTable-component. Anyway, I've brought this to your attention and maybe, by the way, you'll see a solution right away :)

@Rapster
Copy link
Member Author

Rapster commented Aug 20, 2023

Don't wait for this PR to be merged, I don't think it's gonna happen anytime soon (few things must be done before having this merged)

@nimo23
Copy link
Contributor

nimo23 commented Aug 21, 2023

I don't think it's gonna happen anytime soon (few things must be done before having this merged)

No stress. Maybe one day while editing this PR you will casually spot the cause of #9572. Maybe then the problem can be solved at the same time. Just a suggestion. I just wanted to draw your attention to #9572, since you are changing a lot in the Datatable anyway.

@Rapster Rapster added new feature ⚡ performance Performance related issue or enhancement and removed new feature labels Aug 30, 2023
@melloware melloware modified the milestones: 14.0.0, 15.0.0 Jan 3, 2024
@melloware melloware modified the milestones: 14.0.0, 15.0.0 Jan 13, 2024
@Rapster Rapster self-assigned this Mar 25, 2024
@Rapster
Copy link
Member Author

Rapster commented May 3, 2024

@primefaces/primefaces-committers I'm actually thinking of not keeping the backward compatibility here, it's a lot of hassle to make it so for a feature which is not so much used... Hence the importance to make 15 final in 6 months

@melloware
Copy link
Member

@Rapster i agree that to fix this correctly it needs to be refactored as you have mentioned we just have to make the migration guide extremely detailed. I think more people use this feature than we think but who knows! 😄

@Rapster
Copy link
Member Author

Rapster commented May 3, 2024

No you're right, I had look on issues, and col group are used more than I thought... But it doesn't change my previous statement on how tricky it is to make things backward compatible. Hopefully, migration should be easy for people using nested grid with flexgrid (just to name one example)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Change is not backwards compatible enhancement Additional functionality to current component ⚡ performance Performance related issue or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DataTable: frozen columns + column groups not properly rendered Datatable: ColumnGroup refactoring
4 participants