mediamonks / display-temple

Lib
MIT License
0 stars 7 forks source link

Suggestion: wouldn't be better for line-height to be proportional of font-size? #14

Open Syridiana opened 2 years ago

Syridiana commented 2 years ago

https://github.com/mediamonks/display-temple/blob/9cf05ee9b29be9178d93e222123a04ce358a0e02/src/util/fitText.js#L76-L79

As it is now the smaller the font-size the relation between font-size and line-height changes.

As far as I know, is usually a proportional value, like 1.2 of font-size (I believe that is the "auto" value in Photoshop). Also, it would be nice being able to change that ratio through a parameter passed at the call of the function.

mirkovw commented 2 years ago

Hi Yanina, that's a good point! What do you suggest? Instead we could do something like

const lineHeightMultiplier= 1.2 (or override with parameter value from function)
fontSize -= 0.2
lineHeight = fontSize * lineHeightMultiplier;

copyElement.style.fontSize = `${fontSize}px`; 
copyElement.style.lineHeight = `${lineHeight}px`; 

This way, we keep a relative line-height of 1.2 * fontSize

?

alandawi commented 2 years ago

@Syridiana Thanks for opening the topic!

I think it will be an excellent example to contribute to the temple. I hope to see the PR soon! 👀

Syridiana commented 2 years ago

I think we can have a default value, like lineHeightMultiplier = 1.2

and then

fontSize -= 0.2
lineHeight = fontSize * lineHeightMultiplier;

copyElement.style.fontSize = `${fontSize}px`; 
copyElement.style.lineHeight = `${lineHeight}px`; 

And being able to pass an argument to override it.

BUT looking at the signature of fitText function I noticed that is required to include the rest parameter at the end export default function fitText(lineHeightMultiplier = 1.2, ...copyElements) So we will need to call the function in the templates like this: fitText(undefined, [title, ctaCopy]); or fitText(1.2, [title, ctaCopy]);

I don't know if there is an easier workaround.

mm-paulie commented 1 year ago

I think we shouldn't be setting the line-height in javascript. This should be grabbed from the parent element in the CSS what the default is and where it needs to play with. So, if in a project a lineheight of an element is 0.8, it should stay 0.8 since that is belonging to the style, the ratio should by default stay the same as defined in your css since that is part of the looks of it. If some fonts are getting to crippled up together when they are getting smaller, with small lineheights, maybe we should be able to set a minimal line-height for each fitText element instead. With that, you have control on when it needs to stop making the line-height smaller, but still making the font size smaller. This would be element specific, and therefore requires a breaking change for fittext as far as I can see.

We can do something like: fitText([{element:title, minLineHeight:1}, {element:ctaCopy, minLineHeight:1.2}]);