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

[FormControl] Fix label shrink on invalid input value #30424

Closed

Conversation

wladimirguerra
Copy link

@wladimirguerra wladimirguerra commented Dec 28, 2021

Here is the fix. I believe integration tests in different browsers are needed but I don't know how to do it. Does anyone can help me on this?

Closes: #29469

@mui-pr-bot
Copy link

mui-pr-bot commented Dec 28, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 16815b8

@wladimirguerra wladimirguerra force-pushed the input-label-misbehavior branch from 21c642b to 16815b8 Compare December 28, 2021 12:28
@mnajdova
Copy link
Member

mnajdova commented Jan 13, 2022

Doesn't look like it works, I've tested it here in the first TextField - https://codesandbox.io/s/basictextfields-material-demo-forked-99w2m?file=/demo.js

Steps to reproduce:

  1. Click on the first input
  2. Change the value from "2" to "e"
  3. Click outside of the TextField

@wladimirguerra
Copy link
Author

Hey @mnajdova,
Thanks for the reply. I am struggling to run the documentation as dev (yarn && yarn docs:dev or in the root yarn start), so I can look what is going wrong. The change made in TextField is updated, but I need to change the documentation too ( FormPropsTextFields.tsx ) and it is not being updated (even if I run the documentation as dev after the changes). Any help on this?

@danilo-leal danilo-leal added the component: text field This is the name of the generic UI component, not the React module! label Feb 1, 2022
@wladimirguerra
Copy link
Author

Hi there, has been a while that I don't look into this issue. It is clear that the way is to test .validity.badInput. The problem is that the isFilled's obj argument is not always an <input>. Looking deeper in the code now to figure out a solution.

@wladimirguerra
Copy link
Author

Hey @danilo-leal looking into the code it seems that the component is FormControl. It is who manages the filled state. The text field renders a FormControl with its own contexts props.

@danilo-leal
Copy link
Contributor

@wladimirguerra we usually use the text field label for Form Control PRs and issues. I guess it does make sense to create one label for it specifically, especially because it's available in the Base package as well. But anyway, off-topic to the PR :)

@mui-bot
Copy link

mui-bot commented Feb 2, 2022

Details of bundle changes

@material-ui/core: parsed: -3.38% 😍, gzip: -2.90% 😍
@material-ui/lab: parsed: +0.06% , gzip: +0.26%
@material-ui/styles: parsed: -1.61% 😍, gzip: -2.43% 😍
@material-ui/private-theming: parsed: -43.99% 😍, gzip: -42.10% 😍
@material-ui/system: parsed: -1.14% 😍, gzip: -1.58% 😍
@material-ui/unstyled: parsed: +42.12% , gzip: +29.02%
@mui/material-next: parsed: -0.21% 😍, gzip: -0.38% 😍
@mui/joy: parsed: +12.22% , gzip: +9.21%

Generated by 🚫 dangerJS against 70cf808

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 3, 2022
@siriwatknp
Copy link
Member

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch
  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/material/*
  3. run yarn docs:api
    • you might see the changes in docs/pages/material/api/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

@mnajdova
Copy link
Member

Hey @mnajdova,
Thanks for the reply. I am struggling to run the documentation as dev (yarn && yarn docs:dev or in the root yarn start), so I can look what is going wrong. The change made in TextField is updated, but I need to change the documentation too ( FormPropsTextFields.tsx ) and it is not being updated (even if I run the documentation as dev after the changes). Any help on this?

I am very sorry, I have missed this notification.

In the root you should be able to run:

yarn
yarn docs:dev

This will run the docs locally. Before pushing always make sure that you have run:

yarn prettier
yarn proptypes
yarn docs:api
yarn docs:typescript:formatted

These comments should make sure the CI is green. You should also check

yarn lint

@siriwatknp
Copy link
Member

I suggest closing this PR and creating a new one due to the code conflicts are very large (too risky)

@siriwatknp siriwatknp closed this Feb 23, 2022
@wladimirguerra
Copy link
Author

I suggest closing this PR and creating a new one due to the code conflicts are very large (too risky)

Ok @siriwatknp I will do that.

@wladimirguerra
Copy link
Author

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

  1. pull latest master from upstream to your branch

  2. if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
    2.1 revert the change on those markdown files you did
    2.2 pull latest master from upstream (you should not see conflict)
    2.3 make the changes again at docs/data/material/*

  3. run yarn docs:api

    • you might see the changes in docs/pages/material/api/* if your PR touches some of the components
    • it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

I have made changes in ./docs/data/material/components/text-fields/FormPropsTextField.tsx removing the shrink: true from numbers type's TextField, followed the steps proposed, and run yarn docs:dev, but it didn't work. The documentation page didn't change. I guess I am missing a build step or something.

@wladimirguerra
Copy link
Author

I tried to create some test cases, but there is an issue in testing-library that is not easy to work around.

Because of that, I couldn't properly test my changes. I will look a little deeper to find a workaround to the test case.
A help would be well accepted in this point.

Anyway, ASAP when I get things working I will open another PR.

Regards

@mnajdova
Copy link
Member

mnajdova commented Mar 3, 2022

@wladimirguerra feel free to open the PR in draft version and ping someone from the core team to help with the tests if you are stack.

@wladimirguerra wladimirguerra deleted the input-label-misbehavior branch April 12, 2022 19:11
@wladimirguerra
Copy link
Author

Hi there, here is the new attempt to fix the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: text field This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Input text type number misbehavior