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

Tree selection types (pt. 2) #2473

Closed
VsevolodGolovanov opened this issue Nov 26, 2021 · 4 comments · Fixed by #2475
Closed

Tree selection types (pt. 2) #2473

VsevolodGolovanov opened this issue Nov 26, 2021 · 4 comments · Fixed by #2475
Labels
Type: Bug Issue contains a defect related to a specific component.
Milestone

Comments

@VsevolodGolovanov
Copy link

Continuing #2355.

I'm submitting a ... (check one with "x")

[x] bug report
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://forum.primefaces.org/viewforum.php?f=57

Codesandbox Case (Bug Reports)
https://codesandbox.io/s/primereact-tree-selection-types-pr-7-0-1-duvkt

Current behavior

// this doesn't need undefined and TreeSelectionKeysType[]:
type TreeSelectionKeys = string | TreeSelectionKeysType | TreeSelectionKeysType[] | undefined | null;

type TreeSelectionKeyType = boolean | TreeCheckboxSelectionKeyType;

selectionKeys?: TreeSelectionKeys;

// duplicate selectionKeys type, which misses some modes:
interface TreeSelectionKeysType {
    [key: string]: TreeSelectionKeyType;
}

// which is used in onSelectionChange:
interface TreeSelectionParams {
    originalEvent: React.SyntheticEvent;
    value: TreeSelectionKeysType;
}

Expected behavior
So it should be:

type TreeSelectionKeys = string | TreeSelectionKeysType | null;

type TreeSelectionKeyType = boolean | TreeCheckboxSelectionKeyType;

selectionKeys?: TreeSelectionKeys;

// delete TreeSelectionKeysType 

interface TreeSelectionParams {
    originalEvent: React.SyntheticEvent;
    value: TreeSelectionKeys ;
}

Minimal reproduction of the problem with instructions

Please tell us about your environment:

  • React version:

  • PrimeReact version:
    7.0.1

  • Browser: [all | Chrome XX | Firefox XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

  • Language: [all | TypeScript X.X | ES6/7 | ES5]

@melloware
Copy link
Member

@VsevolodGolovanov Can you confirm if you delete the following:

// duplicate selectionKeys type, which misses some modes:
interface TreeSelectionKeysType {
    [key: string]: TreeSelectionKeyType;
}

You still have it being used here without a definition?

type TreeSelectionKeys = string | TreeSelectionKeysType | null;

Am I seeing that right or no?

@VsevolodGolovanov
Copy link
Author

@melloware , oops. I got confused with all the types. TreeSelectionKeysType vs TreeSelectionKeys... Yes, keep TreeSelectionKeysType (maybe rename?), but TreeSelectionParams should use TreeSelectionKeys, not TreeSelectionKeysType.
And TreeSelectionKeys shouldn't have TreeSelectionKeysType[] | undefined.

@VsevolodGolovanov
Copy link
Author

VsevolodGolovanov commented Nov 26, 2021

I'd call TreeSelectionKeysType smth like TreeMultipleSelectionKeys, though it could be confusing since it includes both selectionMode="multiple" and "checkbox". Still an improvement prolly.
Actually it would be better to separate multiple and checkbox:

interface TreeMultipleSelectionKeys {
    [key: string]: boolean;
}
interface TreeCheckboxSelectionKeys {
    [key: string]: TreeCheckboxSelectionKeyType;
}

type TreeSelectionKeys = string | TreeMultipleSelectionKeys | TreeCheckboxSelectionKeys | null;

Because boolean | TreeCheckboxSelectionKeyType makes no sense. You can't have an object with both under different keys.

melloware added a commit to melloware/primereact that referenced this issue Nov 26, 2021
melloware added a commit to melloware/primereact that referenced this issue Nov 26, 2021
@melloware
Copy link
Member

OK check out my PR I agree with you and think I got it right. #2475

melloware added a commit to melloware/primereact that referenced this issue Nov 26, 2021
@mertsincan mertsincan added the Type: Bug Issue contains a defect related to a specific component. label Dec 10, 2021
@mertsincan mertsincan modified the milestones: 8.0.0, 7.1.0 Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issue contains a defect related to a specific component.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants