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

Fixes #9344 #10602

Merged
merged 1 commit into from Oct 3, 2017
Merged

Fixes #9344 #10602

merged 1 commit into from Oct 3, 2017

Conversation

simshaun
Copy link
Contributor

@simshaun simshaun commented Aug 30, 2017

Implements the newer input height formula from #9681 for input groups.

Note 1:

I think it would be nice if the input height formula was moved to a single location so this issue can't reoccur in the future if the formula were to change again. The same formula is now in _input-group.scss, _text.scss, and _select.scss. However, I'm not sure of the best way to do this. Maybe a new file scss/forms/_vars.scss that _input-group.scss, _text.scss, and _select.scss can all use?

Note 2:

I removed the hardcoded height on .input-group-field in non-flexbox-mode so that the input/select/textarea element would just inherit its normal height. There shouldn't be any reason to override the height on .input-group-field unless it's being used on a non input/select/textarea element. I guess this could be considered a breaking change in that scenario. Should I revert?

@kball
Copy link
Contributor

kball commented Sep 9, 2017

@simshaun Thanks for putting this together! For note 1 I definitely agree, and would like to see that as part of this PR. Following the pattern of other components, this would probably be in forms/_forms.scss prior to the imports of the other pieces - that said, I think that is problematic because it makes it impossible to import just one piece. I like your idea of a _vars.scss file, which then could be imported wherever necessary... @ncoden @brettsmason @IamManchanda what do you think?

@rafibomb
Copy link
Member

Yes - Like the idea of a _vars.scss for the common things. Also like that you removed the height on the flex version as it's not needed (flex children inherit same height).

@kball
Copy link
Contributor

kball commented Oct 3, 2017

Going to merge this as a step forward, but would like to see us implement the _vars.scss approach going forward.

@kball kball merged commit 1d203b7 into foundation:develop Oct 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants