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

Fix bug: Line-height issue in [type='file'] #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

maciejpi
Copy link
Contributor

@maciejpi maciejpi commented Dec 1, 2015

@incuna/frontend please merge

It looks like input[type="file"] got there accidentally.

Its sass is in the same file, here:

@grahamgilchrist
Copy link
Contributor

Seems like it was intentionally added to match the size of text inputs. See here:
6c59075#diff-ac0a8f9c204d0a0973a8e3861f58d098R117

What was the reason you needed to remove it?

@maciejpi
Copy link
Contributor Author

maciejpi commented Dec 2, 2015

All the comments there relate to select;
input[type="file"] is set in another place.

Setting line-height to 28px on file input makes the element misaligned
image

After removing the 28px line-height on file input it looks like this:
image

@hippogriffic
Copy link
Contributor

this whole file needs some work really, I have no problem with this being merged

@grahamgilchrist
Copy link
Contributor

The initial comment refers to select and file elements, so that looks to have been intentional.
The sub comments do not mention file elements, but thats probably an error, as they were added at the same time.

There are input[type="file"] properties elsewhere, but that doesn't mean that these aren't necessary. file has likely been grouped with select as one selector because they both share the same CSS properties. The individual file element section does not seem to set height for example.

My concern is that it looks like this was originally added for some sort of cross browser consistency. If we are removing, we should make sure we aren't removing something that is fixing a problem in other projects or browsers?

Maybe we could do some browser testing on a simple test case of a page importing just these form styles, as we could then be sure these initial ones were correct?

@maciejpi
Copy link
Contributor Author

maciejpi commented Dec 7, 2015

@grahamgilchrist I'll do IE testing and will report it back here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants