search

incuna / incuna-sass

Incuna's Sass Library
MIT License
2 stars 2 forks source link

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

Open maciejpi opened 8 years ago

maciejpi commented 8 years ago

@incuna/frontend please merge

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

Its sass is in the same file, here: https://github.com/incuna/incuna-sass/blob/fbf074cf2171e43485148dd8a95dd4f79cb227d3/incuna-sass/components/_forms.sass#L133

grahamgilchrist commented 8 years ago

Seems like it was intentionally added to match the size of text inputs. See here: https://github.com/incuna/incuna-sass/commit/6c590756b01d02620d1a18214080bdb346de7ae1#diff-ac0a8f9c204d0a0973a8e3861f58d098R117

What was the reason you needed to remove it?

maciejpi commented 8 years ago

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 commented 8 years ago

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

grahamgilchrist commented 8 years ago

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 commented 8 years ago

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