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

[TextField] Separate TextFieldHint into separate internal component. #2567

Merged
merged 2 commits into from
Dec 17, 2015

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Dec 17, 2015

This is one of the tasks in #2555. It seeks to simplify the TextField.jsx component by extracting the hint text and its styling logic into its own separate internal component.

Note: I saw this if condition on lines L252-254 that applies a line height to the hint text if a height property is found on the custom root style that is provided. This has not been accounted for, and am wondering if we really want to properly support changing heights of TextFields. If so, we should probably do it differently.

@oliviertassinari oliviertassinari added this to the Composable Components milestone Dec 17, 2015
@oliviertassinari
Copy link
Member

am wondering if we really want to properly support changing heights of TextFields

I don't think so. But I think that we should support various font size.

};

const defaultProps = {
visible: true,
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a typo.
When we have the choice between hidden, show and visible, I would personally go for show.
I also think that show is what it was chosen in some other component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup good catch, and I'll change it to show. I just remembered we did underlineShow in that other pull request, so I'll make it show here.

@newoga
Copy link
Contributor Author

newoga commented Dec 17, 2015

I don't think so. But I think that we should support various font size.

@oliviertassinari The TextField component still has a hintStyle prop where they can customize both font-size and line-height, however that extra code was detecting whether or not you specified a height on the rootStyle that changed the hintStyle for you. I feel like that coupling could be confusing to users and am not a huge fan of that approach but I could reimplement it.

@oliviertassinari
Copy link
Member

I feel like that coupling could be confusing to users and am not a huge fan of that approach

I agree. I hope that we can use the css inheritance of the font size to let people change the font size.
But this feature is tricky to implement. Here is an attend to do so #2039.
I guess that it's not supported by the material spec, where the height of the TextField seems fixed (not linked to the font size neither to the line height).

@newoga
Copy link
Contributor Author

newoga commented Dec 17, 2015

Mmm, that pull request is interesting. I haven't experimented much with the css em/rem stuff thoughtwould be interesting to get something like that working.

I guess that it's not supported by the material spec, where the height of the TextField seems fixed (not linked to the font size neither to the line height)

Agree with you here too.

@oliviertassinari
Copy link
Member

@subjectix I'm ok with this PR. Feel free to merge 👍

alitaheri added a commit that referenced this pull request Dec 17, 2015
[TextField] Separate TextFieldHint into separate internal component.
@alitaheri alitaheri merged commit 36af0e5 into mui:master Dec 17, 2015
@alitaheri
Copy link
Member

@newoga Nice job 👍 👍 Thanks 🎉

@newoga
Copy link
Contributor Author

newoga commented Dec 17, 2015

Thanks!

@newoga newoga deleted the text-field-hint branch December 17, 2015 23:57
@alitaheri alitaheri modified the milestone: Composable Components Dec 23, 2015
@zannager zannager added the component: text field This is the name of the generic UI component, not the React module! label Mar 14, 2023
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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants