cookie-banner icon indicating copy to clipboard operation
cookie-banner copied to clipboard

Document min-height option

Open ilesinge opened this issue 10 years ago • 5 comments

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 :)

ilesinge avatar May 09 '14 09:05 ilesinge

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?

webaddicto avatar May 26 '18 14:05 webaddicto

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

fourroses666 avatar May 26 '18 14:05 fourroses666

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"

AuscanAlliance avatar May 26 '18 14:05 AuscanAlliance

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

fourroses666 avatar May 27 '18 20:05 fourroses666

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?

zytzagoo avatar May 28 '18 07:05 zytzagoo