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

Clean up valuePrepareFunction and filterFunction #117

Closed
uap-universe opened this issue Jan 19, 2023 · 2 comments
Closed

Clean up valuePrepareFunction and filterFunction #117

uap-universe opened this issue Jan 19, 2023 · 2 comments
Assignees
Labels
breaking-change Resolving this issue will introduce a breaking change.
Milestone

Comments

@uap-universe
Copy link
Collaborator

uap-universe commented Jan 19, 2023

The functions that can be specified to control the smart table depending on the row data, all accept the data as input.

Examples:

export type ColumnValuePrepareFunction = (cellValue: any, rowData: any, cell: Cell) => any;
export type ColumnFilterFunction = (cellValue: any, searchString: string, allData: any, cellName: string, rowData: any) => boolean;

Also it is not clear why the filter function accepts so many parameters. Usually only the cell value and the search string should be used to determine the result. The example in the documentation uses rowData to access a field in the data that is not even displayed in the table to apply the filter. This is not realistic in my opinion - no user would ever expect a filter to work like that.
But I can see that there may be a use case where the exact filter implementation may depend on some information about the row - but certainly not about the other rows, so I don't thing the allData is required here.

So the signatures in this example should be:

export type ColumnValuePrepareFunction = (cell: Cell) => string;
export type ColumnFilterFunction = (cell: Cell, searchString: string) => boolean;

We may find more examples of signatures that should be corrected. But this is the most prominent one, I think.

@uap-universe uap-universe added the breaking-change Resolving this issue will introduce a breaking change. label Jan 19, 2023
@uap-universe uap-universe added this to the v3.0.0 milestone Jul 4, 2023
@uap-universe uap-universe self-assigned this Jul 5, 2023
@uap-universe
Copy link
Collaborator Author

Two more observations:

  1. The cell already allows accessing the row, so it's unnecessary to provide both as arguments
  2. The cell uses the valuePrepareFunction. On the other hand, we cannot just remove the cell from the signature, because that definitely breaks backwards compatibility in a very unpleasant manner.

As a compromise, we will define the signatures as follows:

export type ColumnValuePrepareFunction = (rawValue: string, cell: Cell) => string;
export type ColumnFilterFunction = (searchString: string, cell: Cell) => boolean;

uap-universe added a commit that referenced this issue Jul 10, 2023
@uap-universe
Copy link
Collaborator Author

uap-universe commented Jul 11, 2023

relates to #90

Filters apparently do not work on row/cell and instead on the data source, only. Yet another mix-up of responsibilities between table/grid and data source. So the most clean, yet possible signature is:

export type ColumnFilterFunction = (value: string, searchString: string) => boolean;

Of course, this does not provide access to the "row" data anymore. But if that really breaks something, we can add more parameters in 3.x again.

uap-universe added a commit that referenced this issue Aug 8, 2023
Converting the value to a string before applying the valuePrepareFunction somehow defeats the whole purpose of this function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Resolving this issue will introduce a breaking change.
Projects
None yet
Development

No branches or pull requests

1 participant