[TextField] Fix classes forward to InputBase #26231

arpitBhalla · 2021-05-10T10:18:16Z

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

Closes #26157

mui-pr-bot · 2021-05-10T10:20:35Z

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against d4d7a75

mnajdova

Great, thanks!

arpitBhalla · 2021-05-10T11:16:52Z

Great, thanks!

How can I write tests for this?

mnajdova · 2021-05-10T11:32:47Z

How can I write tests for this?

Ideally, we would add in OutlinedInput, Input and FilledInput tests for testing the following:

it('should forward classes to InputBase', () => {
  const { container } = render(<OutlinedInput error classes={{ error: 'error' }} />);
  expect(container.querySelector('.error')).not.to.equal(null);
});

oliviertassinari

What about the types? #20706 (comment)

arpitBhalla · 2021-05-11T05:18:31Z

docs/pages/api-docs/input.json

-    "globalClasses": { "focused": "Mui-focused", "disabled": "Mui-disabled", "error": "Mui-error" },
-    "name": "MuiInput"
-  },
+  "styles": { "classes": [], "globalClasses": {}, "name": null },


Is this correct? 👀

No, looks like something that is not supported in the buildApi. There is an ongoing effort in #25754 for improving this. cc @eps1lon

I think in the mean time we may copy the classes from input base and leave a TODO

I think in the mean time we may copy the classes from input base and leave a TODO

Let's put the brakes on a bit and don't just merge any PR that comes along, yes?

Something is off with the typings. The changed test and this change in the API docs is an indicator for that. Let's investigate this first before merging something we don't understand.

TODOs are fine only if we know what's wrong.

The purpose of the PR is to forward the classes props to the InputBase component so that those can be customized from the other input components FilledInput, Input and OutlinedInput. This is why the types for the classses are extended (and the tests are no longer throwing as this is now supported). The only problem I see is the buildApi not recognizing this syntax.

We could move iteratively, revert the commits after my proposal and leave a TODO. It's already great that we fix a regression. My proposal was to act on the feedback @m4theushw did a year ago. #20706

The only problem I see is the buildApi not recognizing this syntax.

Thanks for being good faith and ignoring the changed test.

The purpose of the PR

Then explain that in the original description not after I point it out. The way PRs are conducted in the recent weeks is not ok. This works for one-off side-projects but I've seen the pain low quality PRs cause. So let's slow down a bit and explain what we're doing. If this isn't possible, then you don't actually understand what a change is doing.

This is why the types for the classses are extended (and the tests are no longer throwing as this is now supported).

Wait, what?

That's definitely not what we want. You shouldn't use props for a variant that you aren't using.

The only problem I see is the buildApi not recognizing this syntax.

Thanks for being good faith and ignoring the changed test.Thanks for being good faith and ignoring the changed test.

You cannot just copy the last sentence and get a conclusion. I have stated before that:

The purpose of the PR is to forward the classes props to the InputBase component so that those can be customized from the other input components FilledInput, Input and OutlinedInput. This is why the types for the classses are extended (and the tests are no longer throwing as this is now supported)

Then explain that in the original description not after I point it out.

The PR was initially suppose the change the classes passed, the typings changes were required afterwards, that's why it's not included in the original description.

why the types for the classses are extended (and the tests are no longer throwing as this is now supported).

Wait, what?

That's definitely not what we want. You shouldn't use props for a variant that you aren't using.

I did not just blindly suggest this. If you see what filledProps is, it has nothing specific to the FilledInput, the inputAdornedStart is supported in InputBase, hence in all Input components. It may have been the case that this logic changed over time, and the test now is not up to date.

packages/material-ui/test/typescript/hoc-interop.spec.tsx

oliviertassinari · 2021-05-15T11:11:18Z

@eps1lon The ambition of the fix was demoted. It's now only fixing the regression (v4 -> v5) on the forward of the classes object, from the wrapper to the InputBase component.

Regarding extending the classes types to support more keys, we have #20706. Is seems that we can remove the "on hold" label as the types are no longer impacted. Do you confirm?

eps1lon · 2021-05-15T12:37:54Z

Regarding extending the classes types to support more keys, we have #20706. Is seems that we can remove the "on hold" label as the types are no longer impacted.

Yep looks ok now after review. In the future, don't blame everything on somebody else on reflex. Thinks usually fail for a reason, not just to block you.

oliviertassinari · 2021-05-16T09:56:35Z

@arpitBhalla Thanks

arpitBhalla · 2021-05-16T09:57:54Z

@oliviertassinari Thanks to you too Sir

added to filled input

9fd49f7

arpitBhalla changed the title ~~[TextField] Fix classes is now forwarded correctly in FilledInput~~ [TextField] Fix classes are now forwarded correctly in FilledInput May 10, 2021

arpitBhalla changed the title ~~[TextField] Fix classes are now forwarded correctly in FilledInput~~ [TextField] Fix classes forward to InputBase May 10, 2021

github-actions bot mentioned this pull request May 10, 2021

Update snapshots for pr/26231 eps1lon/mui-scripts-incubator#2343

Closed

mnajdova approved these changes May 10, 2021

View reviewed changes

Added Tests

f54f916

oliviertassinari added bug 🐛 Something doesn't work component: text field This is the name of the generic UI component, not the React module! labels May 10, 2021

oliviertassinari reviewed May 10, 2021

View reviewed changes

arpitBhalla requested a review from oliviertassinari May 11, 2021 04:57

arpitBhalla commented May 11, 2021

View reviewed changes

eps1lon reviewed May 11, 2021

View reviewed changes

packages/material-ui/test/typescript/hoc-interop.spec.tsx Outdated Show resolved Hide resolved

oliviertassinari mentioned this pull request May 11, 2021

[TextField] Missing pseudo-classes generates error when overriding <Input> #20706

Open

2 tasks

eps1lon added the on hold There is a blocker, we need to wait label May 12, 2021

oliviertassinari approved these changes May 12, 2021

View reviewed changes

[test] We prefer global assertions

d4d7a75

eps1lon removed the on hold There is a blocker, we need to wait label May 15, 2021

oliviertassinari merged commit c5349f1 into mui:next May 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TextField] Fix classes forward to InputBase #26231

[TextField] Fix classes forward to InputBase #26231

arpitBhalla commented May 10, 2021 •

edited

Loading

mui-pr-bot commented May 10, 2021 •

edited

Loading

mnajdova left a comment

arpitBhalla commented May 10, 2021

mnajdova commented May 10, 2021

oliviertassinari left a comment

arpitBhalla May 11, 2021 •

edited

Loading

mnajdova May 11, 2021

eps1lon May 11, 2021 •

edited

Loading

mnajdova May 11, 2021

oliviertassinari May 11, 2021 •

edited

Loading

eps1lon May 12, 2021

eps1lon May 12, 2021 •

edited

Loading

mnajdova May 12, 2021 •

edited

Loading

oliviertassinari commented May 15, 2021 •

edited

Loading

eps1lon commented May 15, 2021

oliviertassinari commented May 16, 2021

arpitBhalla commented May 16, 2021 •

edited

Loading

[TextField] Fix classes forward to InputBase #26231

[TextField] Fix classes forward to InputBase #26231

Conversation

arpitBhalla commented May 10, 2021 • edited Loading

mui-pr-bot commented May 10, 2021 • edited Loading

mnajdova left a comment

Choose a reason for hiding this comment

arpitBhalla commented May 10, 2021

mnajdova commented May 10, 2021

oliviertassinari left a comment

Choose a reason for hiding this comment

arpitBhalla May 11, 2021 • edited Loading

Choose a reason for hiding this comment

mnajdova May 11, 2021

Choose a reason for hiding this comment

eps1lon May 11, 2021 • edited Loading

Choose a reason for hiding this comment

mnajdova May 11, 2021

Choose a reason for hiding this comment

oliviertassinari May 11, 2021 • edited Loading

Choose a reason for hiding this comment

eps1lon May 12, 2021

Choose a reason for hiding this comment

eps1lon May 12, 2021 • edited Loading

Choose a reason for hiding this comment

mnajdova May 12, 2021 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented May 15, 2021 • edited Loading

eps1lon commented May 15, 2021

oliviertassinari commented May 16, 2021

arpitBhalla commented May 16, 2021 • edited Loading

arpitBhalla commented May 10, 2021 •

edited

Loading

mui-pr-bot commented May 10, 2021 •

edited

Loading

arpitBhalla May 11, 2021 •

edited

Loading

eps1lon May 11, 2021 •

edited

Loading

oliviertassinari May 11, 2021 •

edited

Loading

eps1lon May 12, 2021 •

edited

Loading

mnajdova May 12, 2021 •

edited

Loading

oliviertassinari commented May 15, 2021 •

edited

Loading

arpitBhalla commented May 16, 2021 •

edited

Loading