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 #2620: pass standard props to the wrapping element #2708

Merged
merged 2 commits into from
Mar 31, 2022

Conversation

kalinkrustev
Copy link
Contributor

The approach of passing

{...ObjectUtils.findDiffKeys(this.props, ComponentX.defaultProps)}

to the wrapping div/span relies on defaultProps having all the props of ComponentX listed there, otherwise some props will be also passed to the wrapping div/span element. I checked if this is so for some of the components and it look it is.

@kalinkrustev
Copy link
Contributor Author

Fixes #2620

@mertsincan mertsincan merged commit 8df826f into primefaces:master Mar 31, 2022
@mertsincan
Copy link
Member

Good job, @kalinkrustev ;)

@melloware
Copy link
Member

melloware commented Apr 5, 2022

@kalinkrustev i did find one issue upgrading from 7.2.1 to 8.0.0 that maybe you can help me with.

I have a custom component that I wrap PrimeReact AutoComplete like this...

/**
 * Properties for this component which extends PrimeReact AutoComplete.
 */
export interface ActiveDirectoryAutocompleteProps extends AutoCompleteProps {
    value?: ActiveDirectoryUser | ActiveDirectoryUser[];
    suggestions?: ActiveDirectoryUser[];
}

/**
 * JSON object which represents the users in the dropdown.
 */
export interface ActiveDirectoryUser {
    givenName?: string;
    surname?: string;
    jobTitle?: string;
    location?: string;
}

/**
 * Autocomplete for displaying users from Active Directory.
 * 
 * @param {ActiveDirectoryAutocompleteProps} props properties to configure this component
 * @returns {ActiveDirectoryAutocomplete} component
 */
export const ActiveDirectoryAutocomplete = (props: ActiveDirectoryAutocompleteProps) => {

    const selectedItemTemplate = (item: ActiveDirectoryUser) => {
        return `${item.firstName} ${item.lastName}`;
    }

    const itemTemplate = (item: ActiveDirectoryUser) => {
        return (
            <span className="add-user-item flex">
                <div className="flex-initial flex align-items-center justify-content-center">
                    <LetterAvatar fullname={`${item.firstName} ${item.lastName}`} />
                </div>
                <div className="ml-3">
                    <div className="w-full">
                        <span className="block font-bold white-space-nowrap">{item.firstName} {item.lastName}</span>
                    </div>
                    <div style={{ color: "var(--text-color-secondary)" }}>
                        <span className="block text-overflow-ellipsis">{item.email} | {item.jobTitle} | {item.location}</span>
                    </div>
                </div>
            </span>
        );
    }

    return (
        <AutoComplete
            selectedItemTemplate={selectedItemTemplate}
            itemTemplate={itemTemplate}
            delay={400}
            placeholder={!props.value ? "Start typing user name" : ''}
            {...props}
        />
    )
}

However now when I am passing {...props} to do property expansion its failing to compile with... This works in 7.2.1 but now doesn't like it even though above I am extends AutoCompleteProps.

src/components/misc/ActiveDirectoryAutocomplete.tsx:77:10 - error TS2769: No overload matches this call.
  Overload 1 of 2, '(props: AutoCompleteProps | Readonly<AutoCompleteProps>): AutoComplete', gave the following error.
    Type '{ id?: string | undefined; inputRef?: Ref<HTMLInputElement> | undefined; value?: any; name?: string | undefined; type?: string | undefined; suggestions?: any[] | undefined; ... 316 more ...; width?: string | ... 1 more ... | undefined; }' is not assignable to type 'IntrinsicClassAttributes<AutoComplete>'.
      Types of property 'ref' are incompatible.
        Type 'LegacyRef<HTMLSpanElement> | undefined' is not assignable to type 'LegacyRef<AutoComplete> | undefined'.
          Type '(instance: HTMLSpanElement | null) => void' is not assignable to type 'LegacyRef<AutoComplete> | undefined'.
            Type '(instance: HTMLSpanElement | null) => void' is not assignable to type '(instance: AutoComplete | null) => void'.
              Types of parameters 'instance' and 'instance' are incompatible.
                Type 'AutoComplete | null' is not assignable to type 'HTMLSpanElement | null'.
                  Type 'AutoComplete' is missing the following properties from type 'HTMLSpanElement': addEventListener,

Any thoughts?

The change was this

export interface AutoCompleteProps extends Omit<React.DetailedHTMLProps<React.InputHTMLAttributes<HTMLSpanElement>, HTMLSpanElement>, 'onChange' | 'onSelect'> {

@kalinkrustev
Copy link
Contributor Author

I did not try to extend all the components that were modified in this request.
I only followed the pattern from some of the components, that were already passing all properties.
It seems in some (or maybe all the cases) we have to omit the 'ref' property in the definition file.

I tried adding 'ref' to AutoComplete.d.ts:

export interface AutoCompleteProps extends Omit<React.DetailedHTMLProps<React.InputHTMLAttributes<HTMLSpanElement>, HTMLSpanElement>, 'onChange' | 'onSelect' | 'ref'>

And it seems this solves the issue.
I can see 'ref' was omitted in other components too, but I am not that familiar with these tricks.

@kalinkrustev
Copy link
Contributor Author

Meanwhile I think you can do this:

export const ActiveDirectoryAutocomplete = (props: Omit<ActiveDirectoryAutocompleteProps, 'ref'>) => {

Until we figure out a proper fix within Primereact.

@melloware
Copy link
Member

Let me try that!

@melloware
Copy link
Member

The above looks like its working.

@mertsincan
Copy link
Member

Hi @melloware,
Could you please create a new issue about it?

@melloware
Copy link
Member

yep!

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.

3 participants