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

Added some features from the enterprise version #32

Closed
wants to merge 2 commits into from

Conversation

JordanMarr
Copy link
Contributor

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

jwthomson commented Apr 26, 2024

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

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

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
Copy link
Contributor Author

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.

Copy link
Member

@mattgallagher92 mattgallagher92 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few changes that I think we should make. Obviously Compositional IT can make some/all of those, but feel free to make some too @JordanMarr.

type ColumnType = RightAligned | NumericColumn

let openClosed = function | true -> "open" | false -> "closed"

[<ReactComponent>]
let CellRendererComponent<'value,'row> (render:'value -> 'row -> ReactElement, p) =
render p?value p?data
let CellRendererComponent<'row, 'value> (render: (ICellRendererParams<'row, 'value>) -> ReactElement, p: ICellRendererParams<'row, 'value>) =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that this is probably a better interface, but it's a breaking change. We need to consider whether the trade-off is worth it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the style sheet is already a breaking change, maybe let's just bite the bullet and make a v2.

Would be good to add a migration guide in that case (can be minimal, a few sentences in the README or elsewhere might suffice).

//importAll "ag-grid-community/styles/ag-theme-balham.css"
//importAll "ag-grid-community/styles/ag-theme-material.css"

// https://www.ag-grid.com/javascript-data-grid/row-object/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like having the link to the docs easily available. There are two changes I'd consider:

  • Use doc comments, to help library consumers.
  • Always link to the react-data-grid version (rather than the javascript-data-grid one).

startRow : obj
endRow : obj
}
with
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not certain that this is our usual formatting style. Maybe we should get fantomas set up on this codebase and run it?

src/AgGrid.fs Outdated
importAll "ag-grid-community/styles/ag-theme-alpine.css"
importAll "ag-grid-community/styles/ag-theme-balham.css"
importAll "ag-grid-community/styles/ag-theme-material.css"
// User should load the CSS files in their own project
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove these comments and document that users need to do this in their apps. Would be a breaking change.

//static member inline field (v:'a -> string) = columnDefProp<'row, 'value> ("field" ==> v (unbox null))
static member inline field (v:string) = columnDefProp<'row, 'value> ("field" ==> v)
static member inline field (f: 'row -> _) =
// usage: `AgGrid.field _.FirstName` or `AgGrid.field (fun x -> x.FirstName)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good as a doc comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned by others, splitting into separate community and enterprise modules would be worthwhile, particularly to avoid polluting intellisense for community-only users.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to see some of the new features on the demo site. That might pose a problem for the enterprise features if it requires a deployment license. Might be worth contacting AG Grid about - it'd be beneficial for them.

@mattgallagher92
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few more breaking changes that I didn't notice, which can be seen by checking out ./Demo/Components.fs

@mattgallagher92
Copy link
Member

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
Copy link
Contributor Author

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

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
Copy link
Contributor Author

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

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 56db3fc.

The only remaining things to do are:

  • Split the enterprise features into a separate module. I'd suggest aiming for a developer experience where they can open Feliz.AgGrid and open Feliz.AgGrid.Enterprise and access properties in the same way. With those two modules open, ColumnDef.| (where | indicates cursor position) should result in IDE suggestions for both community and enterprise features.
  • Test some of the new features (perhaps in the demo code)

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

@mattgallagher92
Copy link
Member

I've added #34 with the changes we'd like. Closing this PR in favour of that one. Thanks for your contribution @JordanMarr 🙂

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.

5 participants