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.

Sign up for GitHub

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(Dropdown): fix menuRenderer type #2661

Merged
merged 1 commit into from
Dec 18, 2024
Merged

fix(Dropdown): fix menuRenderer type #2661

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/src/components/Dropdown/Dropdown.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ export interface DropdownComponentProps extends CustomMenuBaseProps, CustomOptio
/**
* custom menu render function
*/
menuRenderer?: React.ReactElement;
menuRenderer?: React.ReactElement | ((props: CustomMenuProps) => React.ReactElement);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the type here is a bit more complex
can you check menu.jsx file?
its rendererProps.
everything which is passed to {Renderer && Renderer(rendererProps)}
so apparently we're also passing by mistake innerProps, the onScroll, etc.

what a mess 🤦🏼‍♂️
you can give it an any in the meantime if stuff gets too complicated (see what I did in valueRenderer, they needed the DropdownOption stuff, so I had to add it, but basically DropdownOption is any, so the all props became any at the bundle, so I've worked hard for nothing 🙃)

Copy link
Contributor Author

@rivka-ungar rivka-ungar Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that it is a mess, but isn't it still better to keep it this way then changing it to any?

Copy link
Contributor

Choose a reason for hiding this comment

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

they would have TS issues in the current way if they try to consume something which isn't in your type
we can try that and if they complain we'll release a fix

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 tested it with current implementations and it worked properly, I will keep an eye on it and change if necessary

/**
* Default placement of the Dropdown menu in relation to its control. Use "auto" to flip the menu when there isn't enough space below the control.
*/
Expand Down
Loading