-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
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. |
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. |
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. |
You are welcome! Thanks for providing a nice base to add to. 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. |
There was a problem hiding this 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>) = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/ |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)` |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
There was a problem hiding this 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 more breaking changes that I didn't notice, which can be seen by checking out ./Demo/Components.fs
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. |
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. |
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. |
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:
Those are self-contained, and don't need to be picked up by me (but can be at a later date). |
I've added #34 with the changes we'd like. Closing this PR in favour of that one. Thanks for your contribution @JordanMarr 🙂 |
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).