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

SelectButton: SelectItem[] TypeScript Def #2741

Closed
1 task done
AntonyGarand opened this issue Apr 11, 2022 · 11 comments · Fixed by #2742
Closed
1 task done

SelectButton: SelectItem[] TypeScript Def #2741

AntonyGarand opened this issue Apr 11, 2022 · 11 comments · Fixed by #2742
Assignees
Labels
Component: Documentation Issue or pull request is related to Documentation Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Milestone

Comments

@AntonyGarand
Copy link

AntonyGarand commented Apr 11, 2022

There is no guarantee in receiving an immediate response in GitHub Issue Tracker, If you'd like to secure our response, you may consider PrimeReact PRO Support where support is provided within 4 business hours

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

  • feature request

I'm requesting a feature to be able to style the splitbuttons!

Current behavior
Hey!

At the moment, a split button is wrapper above few buttons, but we can not style the underlying buttons natively, even though buttons have a variety of available styles.
The most obvious missing feature is changing the color, from primary to secondary/warning/any other of the available styles, but other features such as having it be an outlined/raised/rounded buttons are also relevant.

The only way to style buttons at the moment is via CSS, which means duplicating the style we want instead of using the existing class.

image

image

Expected behavior
Being able to style the buttons via one of these methods:

image

@melloware melloware changed the title Supporting class passthrough for SplitButton SelectButton: Supporting class passthrough Apr 11, 2022
@melloware melloware self-assigned this Apr 11, 2022
@melloware melloware added the Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add label Apr 11, 2022
@melloware melloware added this to the 8.0.0-rc.2 milestone Apr 11, 2022
@melloware
Copy link
Member

I will take this one.

@melloware
Copy link
Member

@AntonyGarand it looks like you can already do this on the options but setting className on each option.

So if you make your options like...

const citySelectItems = [
    {label: 'New York', value: 'NY', className: 'my-class-name`},
    {label: 'Rome', value: 'RM', className: 'my-class-name`},
    {label: 'London', value: 'LDN', className: 'my-class-name`},
    {label: 'Istanbul', value: 'IST', className: 'my-class-name`},
    {label: 'Paris', value: 'PRS', className: 'my-class-name`}
];

here is the source where its creating each button.

 const createItems = () => {
        if (props.options && props.options.length) {
            return props.options.map((option, index) => {
                const isDisabled = props.disabled || isOptionDisabled(option);
                const optionLabel = getOptionLabel(option);
                const tabIndex = isDisabled ? null : 0;
                const selected = isSelected(option);
                const key = optionLabel + '_' + index;

                return <SelectButtonItem key={key} label={optionLabel} className={option.className} option={option} onClick={onOptionClick} template={props.itemTemplate}
                    selected={selected} tabIndex={tabIndex} disabled={isDisabled} ariaLabelledBy={props.ariaLabelledBy} />
            });
        }

        return null;
    }

Can you try it and verify?

@melloware
Copy link
Member

here is the TypeScript def for SelectItem

export interface SelectItem {
    label?: string;
    value?: any;
    className?: string;
    icon?: IconType<SelectItem>;
    title?: string;
    disabled?: boolean;
}

@AntonyGarand
Copy link
Author

It does indeed work!
Looks like the SelectButtonProps options does not specify a SelectItem type, using any[] instead.
Since it's not documented on the docs either, I assumed it didn't support it, oops!

Thanks for the quick answer!

export interface SelectButtonProps {
    id?: string;
    value?: any;
    options?: any[];
    // ...

Not sure if I should leave this open and change it to a feedback to update the docs / ts or close it though.

@melloware
Copy link
Member

Let me ask @mertsincan should I change the TS definition from any[] to SelectItem[] ??

@melloware melloware assigned mertsincan and unassigned melloware Apr 11, 2022
@melloware melloware added the Component: Documentation Issue or pull request is related to Documentation label Apr 11, 2022
@AntonyGarand
Copy link
Author

I believe it should be a mix of both: It can have more properties than SelectItem, especially if using a template for the items, but should at least have partial support for SelectItem.
Perhaps something like SelectItem & any to be as generic as possible

@melloware
Copy link
Member

I submitted a PR not sure if it is correct or not though!

@melloware melloware changed the title SelectButton: Supporting class passthrough SelectButton: SelectItem[] TypeScript Def Apr 12, 2022
melloware added a commit to melloware/primereact that referenced this issue Apr 12, 2022
mertsincan pushed a commit that referenced this issue Apr 15, 2022
* Fix #2741: SelectButton options TypeScript def

* Fix #2741: SelectButton options TypeScript def
@1EDExg0ffyXfTEqdIUAYNZGnCeajIxMWd2vaQeP

SelectItem[] | any[] provides literally no benefit over any[] with regards to typechecking, as described in dtslint. The type SelectItem already included the case that it may have more properties, so there's no need to use a union type here.

@melloware
Copy link
Member

Feel free to submit a PR!

@AntonyGarand
Copy link
Author

It might not have any difference in regards to typechecking, but it does help the humans figure out the expected types, which caused this issue overall.
I'd vote to keep it with the union type for this purpose: No downsides into keeping it as-is either.

@1EDExg0ffyXfTEqdIUAYNZGnCeajIxMWd2vaQeP

But is there any reason one wouldn't pass a SelectItem[]?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Documentation Issue or pull request is related to Documentation Type: Enhancement Issue contains an enhancement related to a specific component. Additional functionality has been add
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants