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

[Input][joy-ui] Display decorators when passed in slots #38295

Closed
wants to merge 4 commits into from

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Aug 3, 2023

@sai6855 sai6855 added bug 🐛 Something doesn't work package: joy-ui Specific to @mui/joy component: input labels Aug 3, 2023
@mui-bot
Copy link

mui-bot commented Aug 3, 2023

Netlify deploy preview

https://deploy-preview-38295--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5f99152

@sai6855 sai6855 marked this pull request as draft August 3, 2023 07:46
@sai6855 sai6855 marked this pull request as ready for review August 3, 2023 08:33
@sai6855 sai6855 changed the title [Joy][Input] Fix decorators when passed in slots [Joy][Input] Display decorators when passed in slots Aug 3, 2023
@oliviertassinari oliviertassinari changed the title [Joy][Input] Display decorators when passed in slots [Input][joy-ui] Display decorators when passed in slots Aug 3, 2023
Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

It's fine for a fix, but we can likely re-think if we need both APIs or not. cc @siriwatknp

@@ -334,12 +334,14 @@ const Input = React.forwardRef(function Input(inProps, ref) {

return (
<SlotRoot {...rootProps}>
{startDecorator && (
{(startDecorator ?? startDecoratorProps.ownerState.slots?.startDecorator) && (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{(startDecorator ?? startDecoratorProps.ownerState.slots?.startDecorator) && (
{(startDecorator ?? slots?.startDecorator) && (

Copy link
Contributor Author

@sai6855 sai6855 Aug 7, 2023

Choose a reason for hiding this comment

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

fixed in this commit 885f0a9

<SlotStartDecorator {...startDecoratorProps}>{startDecorator}</SlotStartDecorator>
)}

<SlotInput {...inputProps} />
{endDecorator && <SlotEndDecorator {...endDecoratorProps}>{endDecorator}</SlotEndDecorator>}
{(endDecorator ?? endDecoratorProps.ownerState.slots?.endDecorator) && (
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{(endDecorator ?? endDecoratorProps.ownerState.slots?.endDecorator) && (
{(endDecorator ?? slots?.endDecorator) && (

@sai6855 sai6855 requested a review from mnajdova August 7, 2023 10:27
@@ -334,12 +334,14 @@ const Input = React.forwardRef(function Input(inProps, ref) {

return (
<SlotRoot {...rootProps}>
{startDecorator && (
{(startDecorator ?? slots?.startDecorator) && (
Copy link
Member

Choose a reason for hiding this comment

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

I think we should not do this. The intention of slots.* is to replace the default slot component, there shouldn't be other logic involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, should we add something in docs explaining same? I think it's confusing right now

Copy link
Member

Choose a reason for hiding this comment

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

Agree!

Copy link
Member

@siriwatknp siriwatknp Sep 1, 2023

Choose a reason for hiding this comment

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

We already have the overall doc about slots at https://mui.com/joy-ui/customization/overriding-component-structure/.

We could add a note on the Input page under the decorator section.

@siriwatknp
Copy link
Member

See my comment on why we should drop this PR (feel free to comment your thought as well)

@sai6855
Copy link
Contributor Author

sai6855 commented Sep 1, 2023

See my comment on why we should drop this PR (feel free to comment your thought as well)

#38295 (comment)

@sai6855 sai6855 closed this Sep 13, 2023
@oliviertassinari oliviertassinari added the component: text field This is the name of the generic UI component, not the React module! label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Input] slots overrides for startDecorator and endDecorator are not populated
5 participants