search

CompositionalIT / feliz-ag-grid

9 stars 4 forks source link

Added some features from the enterprise version #32

Open JordanMarr opened 2 months ago

JordanMarr commented 2 months ago

As promised, here is a PR with a ton of mostly enterprise features I needed for my client project.

I think there may be one or two small breaking changes (I can't remember now).  

jwthomson commented 2 months ago

Thanks for submitting this PR, overall it looks good to me.

I'm not sure if there's much benefit to leaving in the various comments for sections which have been removed - for instance where we are no longer loading in CSS files, and functions where overloading was breaking type inference.

@mattgallagher92 do you have any more specific feedback for this PR?

The only other thing that I was thinking is, do we definitely want to have a mix of all the free and enterprise features in the same place? I think it is probably fine but worth mentioning in case I haven't thought of something.

Larocceau commented 2 months ago

Without looking in-depth at the changes in the PR, I don't think we should mix together the free and enterprise features. I think a separate module in the the same project should do.

mattgallagher92 commented 2 months ago

Thanks @JordanMarr!

@jwthomson I agree that we should remove the commented out lines; maybe we should update the usage docs and/or release notes to make it clear that users are required to add these imports now.

I'm afraid that I don't have time for an in-depth review right now. Hopefully next week! Looks good at a glance though.

@Larocceau I agree that a separate module is probably a good idea. We can either do that now, or merge the PR and restructure the repo before publishing a new version of the package.

JordanMarr commented 2 months ago

You are welcome! Thanks for providing a nice base to add to. I have other changes already, but I'll wait for the dust to settle before submitting anything else. The AgGrid API is so vast. Even with all the stuff I added, it's just a drop in the bucket compared to how much is in the API.

I think that https://github.com/glutinum-org/Glutinum by @MangelMaxime would be a nice way to generate all the API objects, but I was too busy trying to manually plug in the stuff I needed to go down that path. But I imagine that it would certainly add some consistency if all those objects could just be generated.

mattgallagher92 commented 1 month ago

Sorry for this dragging on, we're a bit thin on the ground at the moment. I've blocked out some time to address the feedback next Friday.

mattgallagher92 commented 1 month ago

I've started implementing some of the changes at https://github.com/CompositionalIT/feliz-ag-grid/tree/JordanMarr/main (I couldn't push to Jordan's repo). I'm not going to get any further today.

JordanMarr commented 1 month ago

I know the enterprise stuff is yet to be separated, but I just added the LicenseManager in here for now so that nobody else has to scratch their head on getting the binding to work properly as I just did.

image

isaacabraham commented 2 weeks ago

Hey @JordanMarr - just wanted to apologise for not getting this PR over the line yet. We were planning on doing that today but something has come up that's pushed it back. We are definitely keen on getting this in - I think that you can consider it "approved in principle" - it's just for us to go through and make a few more tweaks before merging.

Thank you so much for doing this.

JordanMarr commented 2 weeks ago

No need to apologize. I'm happy to contribute back. There is no rush on my side as I will keep a copy of the bindings in my project until the dust settles.

mattgallagher92 commented 4 days ago

I've updated https://github.com/CompositionalIT/feliz-ag-grid/tree/JordanMarr/main to include Jordan's latest change, and have fixed a bunch of the issues that I mentioned.

One regression that I didn't notice during review, but noticed while fixing the demo code, was that we lost some type safety and type inference amongst the changes to the API. I've fixed the regression in 56db3fc1563372630ac03458b6c5d02a01387cb4.

The only remaining things to do are:

Those are self-contained, and don't need to be picked up by me (but can be at a later date).