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: column.editor and gridOptions.editorFactory type changed #995

Merged
merged 7 commits into from
Mar 11, 2024
Merged

fix: column.editor and gridOptions.editorFactory type changed #995

merged 7 commits into from
Mar 11, 2024

Conversation

manfredwippel
Copy link
Contributor

editor and edtitor factory do not return an Editor Object but a typeof Editor to be constructed with new (...) Otherwise it is impossible to implement an Editor Factory in Typescript

editor and edtitor factory do not return an Editor Object but a typeof Editor to be constructed with new (...)
Otherwise it is impossible to implement an Editor Factory in Typescript
src/models/column.interface.ts Outdated Show resolved Hide resolved
src/models/editor.interface.ts Outdated Show resolved Hide resolved
src/slick.grid.ts Outdated Show resolved Hide resolved
src/slick.grid.ts Show resolved Hide resolved
src/slick.grid.ts Outdated Show resolved Hide resolved
src/slick.grid.ts Show resolved Hide resolved
src/slick.grid.ts Outdated Show resolved Hide resolved
src/slick.grid.ts Outdated Show resolved Hide resolved
@ghiscoding
Copy link
Collaborator

It would also be nice if you could provide code sample of how you expect to use this as an Editor Factory like you mentioned. Thanks

@ghiscoding ghiscoding changed the title column.editor and gridOptions.editorFactory type changed fix: column.editor and gridOptions.editorFactory type changed Mar 7, 2024
@manfredwippel manfredwippel requested a review from ghiscoding March 8, 2024 06:17
@manfredwippel
Copy link
Contributor Author

It would also be nice if you could provide code sample of how you expect to use this as an Editor Factory like you mentioned. Thanks

I would provide an example and tests after I figured out a proper solution.
I am currently implementing Slick Grid as Serverside rendered control for our project and would create clean JSON without javascript references. Therefore I would need to extend Column Metadata and Editor functionality to define editors, formaters, validators and enable conditions per definition.

src/slick.grid.ts Outdated Show resolved Hide resolved
@manfredwippel manfredwippel requested a review from ghiscoding March 9, 2024 09:57
@ghiscoding
Copy link
Collaborator

@manfredwippel I found 2 more lines to update, if you could please fix them, we could then go ahead with the merge. Thanks

manfredwippel and others added 2 commits March 11, 2024 07:29
Co-authored-by: Ghislain B. <gbeaulac@gmail.com>
Co-authored-by: Ghislain B. <gbeaulac@gmail.com>
@@ -1,6 +1,7 @@
import type { EditorArguments } from './editorArguments.interface';
import type { EditorValidationResult } from './editorValidationResult.interface';

import type { GridOption } from '../models/gridOption.interface';
Copy link
Collaborator

Choose a reason for hiding this comment

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

after the CI ran I see that the build is failing, 1 of the reason is that you have duplicate import, you can remove line 3 since GridOption is imported in line 4 as well so you have duplicate imports.

@ghiscoding
Copy link
Collaborator

@manfredwippel the build is failing in the UI, I have identified at least 1 duplicate import as shown above. The CI runs the build:prod script as shown below, so please make sure you run it too npm run build:prod

"build:prod": "node ./scripts/builds.mjs --prod",

@@ -2,8 +2,9 @@
import { Column, ElementPosition, PositionMethod } from './index';
import type { SlickDataView } from '../slick.dataview';
import type { SlickGrid } from '../slick.grid';
import type { Column, ElementPosition, GridOption, PositionMethod } from './index';
Copy link
Collaborator

Choose a reason for hiding this comment

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

more duplicate imports since line 2 and 5 are nearly the same, line 5 seems better to keep since it adds the missing type which is good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am sorry - I did not run a build since I accepted your changes in the browser only and did not sync.

Copy link
Collaborator

Choose a reason for hiding this comment

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

np

@ghiscoding ghiscoding merged commit e02cf64 into 6pac:master Mar 11, 2024
2 checks passed
@ghiscoding
Copy link
Collaborator

ok it looks good now, let's merge. I'm not sure when I'll push a new release but sometime this week I guess, if not please remind me

@ghiscoding
Copy link
Collaborator

ghiscoding commented Mar 12, 2024

@manfredwippel I have replicated your PR into my lib Slickgrid-Universal and I found a small problem, you've assigned EditorArguments as optional and possibly undefined to the EditorConstructor but that's technically impossible because without it, the Editor simply won't work or do anything since it gets all its data through that argument. So would you like to create another PR to resolve this issue?

export type EditorConstructor = {
new <TData = any, C extends Column<TData> = Column<TData>, O extends GridOption<C> = GridOption<C>>(args?: EditorArguments<TData, C, O>): Editor;

@ghiscoding
Copy link
Collaborator

now available in v5.8.2, thanks for the contribution

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.

2 participants