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]: add a compact property that reduce height #7109

Closed
wants to merge 1 commit into from

Conversation

MichaelMure
Copy link
Contributor

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

This PR add a compact option to the TextField to optionally reduce the height. Does that make sense ?

Disclaimer:
giphy-downsized-large

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 12, 2017

Hey, thanks for that PR. I think that we know have enough feedback to change the height behaviour of the TextField introduced in #6566 by @kybarg. I don't think that we should expose a compact option, at first, I thought it was referring to the dense modifier of the specification. Also, I don't think that we need a new demo component. That's creating noise.

What do you about the following?

  • We use a consistent top and bottom spacing between the floating text and non-floating text mode.
  • We remove the bottom margin of 8px right under the ink bar.
  • We remove the top margin
  • We add an optional margin boolean property false by default to add 8px margin-bottom and 16px margin-top a bit like the polymer element:

capture d ecran 2017-06-12 a 13 01 31

cc @jgoux, @MichaelMure as you raised that issue too.

@oliviertassinari oliviertassinari added component: text field This is the name of the generic UI component, not the React module! next PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jun 12, 2017
@jgoux
Copy link
Contributor

jgoux commented Jun 12, 2017

I'm all for this change. 👍

I'm not even sure if adding a margin property is required.
I think this concern should be in a parent component, or maybe defining these margins in a "form" object in the theme so users can use "standard" values.

@MichaelMure
Copy link
Contributor Author

words

If you don't mind, I'll watch and learn.

@oliviertassinari
Copy link
Member

@MichaelMure Sounds good.

I'm closing for #7113.

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: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants