[InputUnstyled] Fix externally provided `inputRef` is ignored #35807

sai6855 · 2023-01-12T09:55:57Z

I have followed (at least) the PR section of the contributing guide.

mui-bot · 2023-01-12T09:59:21Z

	Messages
📖	Netlify deploy preview: https://deploy-preview-35807--material-ui.netlify.app/

Details of bundle changes

Generated by 🚫 dangerJS against 17457f5

packages/mui-base/src/InputUnstyled/InputUnstyled.test.tsx

packages/mui-base/src/InputUnstyled/InputUnstyled.tsx

packages/mui-base/src/InputUnstyled/InputUnstyled.test.tsx

Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>

ZeeshanTamboli

Thanks @sai6855 ! Looks good!

sai6855 · 2023-01-14T07:33:25Z

Thanks for the review!!

mnajdova

Shouldn't this prop comes from the slotProps.input.ref instead? cc @michaldudak

michaldudak · 2023-01-16T11:28:43Z

Yes, this was a general idea. In the case of the Unstyled Input, I suspect that using a ref to the underlying input may be pretty common, so we may additionally expose it in the form of inputRef in the future, but let's use slotProps.input.ref for now.

packages/mui-base/src/InputUnstyled/InputUnstyled.tsx

sai6855 · 2023-01-16T12:08:34Z

packages/mui-base/src/InputUnstyled/InputUnstyled.tsx

+      typeof slotProps.input === 'object' && typeof slotProps.input.ref !== 'undefined'
+        ? slotProps.input.ref
+        : undefined,


this is the best i could do to make typescript happy

why do you need to pass ref from slotPros.input.ref? It will be forked in useSlotProps for Input.

why do you need to pass ref from slotPros.input.ref?

If someone wants to provide their own ref to the InputUnstyled, they'd do it with the slotPros.input.ref.

this is the best i could do to make typescript happy

Ah, I see, we've got a bit of a chicken and egg problem here - slotProps possibly need ownerState, which needs useInput result, which depend on slotProps. We have to do this differently, then. Do not pass inputRef to useInput, but add it to input slot's useSlotProps' additionalProps object. The useSlotProps hook should take care of merging it correctly with the result of useInput.

So it turns out we don't need to pass slotProps.input.ref in additional props. externalProps (i guess) in the same hook is taking care of passing ref to useInput. All we have to do is destructure ref and fork :)

Added test to make sure input element is attached to ref.

https://github.com/sai6855/material-ui/blob/79b43f90054c17382e596e4eee55034872d07611/packages/mui-base/src/InputUnstyled/InputUnstyled.test.tsx#L32-L39

Indeed! I forgot what functionality I added to useSlotProps ;)

sai6855 · 2023-01-16T12:22:51Z

@ZeeshanTamboli @michaldudak updated code as suggested.

ZeeshanTamboli · 2023-01-16T12:27:30Z

Yes, this was a general idea. In the case of the Unstyled Input, I suspect that using a ref to the underlying input may be pretty common, so we may additionally expose it in the form of inputRef in the future, but let's use slotProps.input.ref for now.

@mnajdova @michaldudak So then, implementation-wise, should we remove the inputRef type from UseInputParameters? That would be a breaking change. Or should we rather consider both, with the slot taking precedence?

inputRef: slotProps.input?.ref ?? inputRefProp

mnajdova · 2023-01-19T14:52:14Z

@mnajdova @michaldudak So then, implementation-wise, should we remove the inputRef type from UseInputParameters? That would be a breaking change. Or should we rather consider both, with the slot taking precedence?

Remove it from the interface, it was anyway not handled, so I would say it was broken even before. We can anyway list is a breaking change, because of the type change.

ivp-dev · 2023-01-19T15:01:50Z

@mnajdova @michaldudak So then, implementation-wise, should we remove the inputRef type from UseInputParameters? That would be a breaking change. Or should we rather consider both, with the slot taking precedence?

Remove it from the interface, it was anyway not handled, so I would say it was broken even before. We can anyway list is a breaking change, because of the type change.

It is extremly useful feature for developers who create custom components based on useInput hook because in other case you have no any possibility to get ref for input element but to call getInputProps. Maybe it is better continue support it, just handle ref inside the hook?

michaldudak · 2023-01-19T15:08:08Z

We can leave the inputRef as a hook parameter, but not include it in the component props.
Whatever is set in InputUnstyled's slotProps.input.ref, should be passed to useInput's inputRef.
Then, useInput will merge the incoming ref with the internal one.

sai6855 · 2023-01-19T15:20:21Z

We can leave the inputRef as a hook parameter, but not include it in the component props. Whatever is set in InputUnstyled's slotProps.input.ref, should be passed to useInput's inputRef. Then, useInput will merge the incoming ref with the internal one.

Then I guess no changes is required for PR, current state of PR does exactly as you described

michaldudak · 2023-01-19T16:07:10Z

Actually, as I commented in #35807 (comment), slotProps.input.ref can't be passed to useInput as it may not be known at the time (useSlotProps can be a function).

ivp-dev · 2023-01-19T17:33:47Z

Please, check PR

sai6855 · 2023-01-19T19:21:29Z

packages/mui-base/src/InputUnstyled/InputUnstyled.test.tsx

+  it('should be able to attach input ref passed through props', () => {
+    const inputRef = React.createRef<HTMLInputElement>();
+    const { getByRole } = render(<InputUnstyled slotProps={{ input: { ref: inputRef } }} />);
+    expect(getByRole('textbox')).to.deep.equal(inputRef.current);


Just to make sure input element is attached to ref.

previously test is written as
expect(inputRef.current).not.to.equal(null)

We could use one more test to verify the behavior of useInput alone. See https://github.com/mui/material-ui/pull/35880/files#r1082379077

packages/mui-base/src/InputUnstyled/useInput.test.tsx

michaldudak · 2023-01-20T12:02:15Z

packages/mui-base/src/InputUnstyled/useInput.test.tsx

+      }
+      const { getByRole } = render(<Input />);
+
+      expect(getByRole('textbox')).to.deep.equal(inputRef.current);


This should be the other way around:

Suggested change

expect(getByRole('textbox')).to.deep.equal(inputRef.current);

expect(inputRef.current).to.deep.equal(getByRole('textbox'));

michaldudak

Thanks!

sai6855 added 2 commits January 12, 2023 15:23

Make ref accessable

Verified

This commit was signed with the committer’s verified signature.

LukasTy Lukas Tyla

GPG key ID: A833C9AF81A66D8C

Verified
Learn about vigilant mode

9a6fee1

patch

9bdde11

sai6855 changed the title ~~[InputUnstyled] Make ref passes through accessable~~ [InputUnstyled] Attach value to ref passed through props Jan 12, 2023

proptypes

0549d09

ZeeshanTamboli added package: base-ui component: text field bug 🐛 labels Jan 12, 2023

trigger ci

555d43d

sai6855 mentioned this pull request Jan 12, 2023

[useInput] Externally provided ref is ignored #35783

Closed

ZeeshanTamboli reviewed Jan 14, 2023

View reviewed changes

packages/mui-base/src/InputUnstyled/InputUnstyled.test.tsx Outdated Show resolved Hide resolved

packages/mui-base/src/InputUnstyled/InputUnstyled.tsx Outdated Show resolved Hide resolved

packages/mui-base/src/InputUnstyled/InputUnstyled.tsx Outdated Show resolved Hide resolved

changes requested

84d8ee9

ZeeshanTamboli reviewed Jan 14, 2023

View reviewed changes

packages/mui-base/src/InputUnstyled/InputUnstyled.test.tsx Outdated Show resolved Hide resolved

ZeeshanTamboli added 2 commits January 14, 2023 12:17

Update packages/mui-base/src/InputUnstyled/InputUnstyled.test.tsx

00a001d

Signed-off-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com>

sort prop

547dad2

ZeeshanTamboli approved these changes Jan 14, 2023

View reviewed changes

ZeeshanTamboli changed the title ~~[InputUnstyled] Attach value to ref passed through props~~ [InputUnstyled] Fix externally provided inputRef is ignored Jan 14, 2023

mnajdova reviewed Jan 16, 2023

View reviewed changes

sai6855 added 2 commits January 16, 2023 17:30

moved inputRef to slotProps

d956088

removed comment

15dca1a

sai6855 commented Jan 16, 2023

View reviewed changes

packages/mui-base/src/InputUnstyled/InputUnstyled.tsx Outdated Show resolved Hide resolved

sai6855 commented Jan 16, 2023

View reviewed changes

sai6855 added 3 commits January 20, 2023 00:24

moved ref to useSlotProps

2b07fd9

patch

ac4c42d

modify test

79b43f9

sai6855 commented Jan 19, 2023

View reviewed changes

sai6855 requested a review from michaldudak January 19, 2023 19:37

michaldudak mentioned this pull request Jan 20, 2023

[Base][useInput]#35783 fix useinput ref #35880

Closed

1 task

sai6855 added 2 commits January 20, 2023 16:54

added useinput test

57ba58f

update import

700e04f

sai6855 commented Jan 20, 2023

View reviewed changes

packages/mui-base/src/InputUnstyled/useInput.test.tsx Show resolved Hide resolved

michaldudak reviewed Jan 20, 2023

View reviewed changes

sai6855 added 3 commits January 20, 2023 17:34

swap

9700140

swap

bc94045

prettier

17457f5

sai6855 requested a review from michaldudak January 20, 2023 12:37

michaldudak approved these changes Jan 20, 2023

View reviewed changes

michaldudak merged commit 4aeda27 into mui:master Jan 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[InputUnstyled] Fix externally provided `inputRef` is ignored #35807

[InputUnstyled] Fix externally provided `inputRef` is ignored #35807

sai6855 commented Jan 12, 2023 •

edited

Loading

mui-bot commented Jan 12, 2023 •

edited

Loading

ZeeshanTamboli left a comment

sai6855 commented Jan 14, 2023

mnajdova left a comment

michaldudak commented Jan 16, 2023

sai6855 Jan 16, 2023

ivp-dev Jan 17, 2023 •

edited

Loading

michaldudak Jan 19, 2023

sai6855 Jan 19, 2023 •

edited

Loading

michaldudak Jan 20, 2023

sai6855 commented Jan 16, 2023

ZeeshanTamboli commented Jan 16, 2023 •

edited

Loading

mnajdova commented Jan 19, 2023

ivp-dev commented Jan 19, 2023

michaldudak commented Jan 19, 2023 •

edited

Loading

sai6855 commented Jan 19, 2023 •

edited

Loading

michaldudak commented Jan 19, 2023

ivp-dev commented Jan 19, 2023 •

edited

Loading

sai6855 Jan 19, 2023 •

edited

Loading

michaldudak Jan 20, 2023

sai6855 Jan 20, 2023

michaldudak Jan 20, 2023

sai6855 Jan 20, 2023

michaldudak left a comment

	expect(getByRole('textbox')).to.deep.equal(inputRef.current);
	expect(inputRef.current).to.deep.equal(getByRole('textbox'));

[InputUnstyled] Fix externally provided inputRef is ignored #35807

[InputUnstyled] Fix externally provided inputRef is ignored #35807

Conversation

sai6855 commented Jan 12, 2023 • edited Loading

mui-bot commented Jan 12, 2023 • edited Loading

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

sai6855 commented Jan 14, 2023

mnajdova left a comment

Choose a reason for hiding this comment

michaldudak commented Jan 16, 2023

sai6855 Jan 16, 2023

Choose a reason for hiding this comment

ivp-dev Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

michaldudak Jan 19, 2023

Choose a reason for hiding this comment

sai6855 Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

michaldudak Jan 20, 2023

Choose a reason for hiding this comment

sai6855 commented Jan 16, 2023

ZeeshanTamboli commented Jan 16, 2023 • edited Loading

mnajdova commented Jan 19, 2023

ivp-dev commented Jan 19, 2023

michaldudak commented Jan 19, 2023 • edited Loading

sai6855 commented Jan 19, 2023 • edited Loading

michaldudak commented Jan 19, 2023

ivp-dev commented Jan 19, 2023 • edited Loading

sai6855 Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

michaldudak Jan 20, 2023

Choose a reason for hiding this comment

sai6855 Jan 20, 2023

Choose a reason for hiding this comment

michaldudak Jan 20, 2023

Choose a reason for hiding this comment

sai6855 Jan 20, 2023

Choose a reason for hiding this comment

michaldudak left a comment

Choose a reason for hiding this comment

[InputUnstyled] Fix externally provided `inputRef` is ignored #35807

[InputUnstyled] Fix externally provided `inputRef` is ignored #35807

sai6855 commented Jan 12, 2023 •

edited

Loading

mui-bot commented Jan 12, 2023 •

edited

Loading

ivp-dev Jan 17, 2023 •

edited

Loading

sai6855 Jan 19, 2023 •

edited

Loading

ZeeshanTamboli commented Jan 16, 2023 •

edited

Loading

michaldudak commented Jan 19, 2023 •

edited

Loading

sai6855 commented Jan 19, 2023 •

edited

Loading

ivp-dev commented Jan 19, 2023 •

edited

Loading

sai6855 Jan 19, 2023 •

edited

Loading