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

Document min-height option #9

Open
ilesinge opened this issue May 9, 2014 · 5 comments
Open

Document min-height option #9

ilesinge opened this issue May 9, 2014 · 5 comments

Comments

@ilesinge
Copy link
Contributor

ilesinge commented May 9, 2014

The data-min-height option should be documented in README.md.
I would have done it via a pull-request, but I just don't understand the point of this option...
Thanks :)

@webaddicto
Copy link
Contributor

Maybe we can remove minHeight option?

I notice that if we use data-min-height we set also line-height to same minHeight value:

el.style.lineHeight = el.style.minHeight;

Isn't this bad? Because if we set a minHeight of 50px we'll end up having a line-height of 50px and on mobile it looks very bad:

mobile

I personally think we should allow user to set custom line-height and remove minHeight.

We can set banner height using data-padding="20px 20px".

What are your thought?

@fourroses666
Copy link

I overwrite it with css. Also use × for the close button althow it might be nice to have a real button with green bg color

@AuscanAlliance
Copy link

Green Button simple but works for me - I use... data-close-text="Got It!" data-close-style="background:#4CAF50;float:right;color:white;padding:2px 12px;text-align:center;display:inline-block;margin-left:10px"

@fourroses666
Copy link

fourroses666 commented May 27, 2018

Thanks @AuscanAlliance , nice solution.

I agree on leaving the height, don't use line-height for the height but padding (top bot) so when text goes over 2 lines it will be looking nice.

I see data-padding="20px 20px" is possible

@zytzagoo
Copy link
Collaborator

The min-height option (via data-min-height) is (still) not documented for a reason :)

I don't know it's original purpose, since it landed with PR #1 -- it mentions responsiveness in the title and that's it :/

From a maintenance/release point of view, removing anything visually-related (such as minHeight, which also sets lineHeight unfortunately) will probably cause someone somewhere to not have identical output visually when they upgrade. So, that means at least a major version bump + maybe some deprecation notice and whatnot... Bleh. Can we prove somehow that removing minHeight option will not result in visual changes? (Or can we ensure the same effect some other way?)

Setting the height is already possible, and since recently so is padding.

Setting line-height via option would also be easy to do, right? Would that solve the problem with line-height + min-height, while also leaving min-height as is?

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

No branches or pull requests

5 participants