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

improve font-size consistency with textMarginRatio property #134

Merged

Conversation

GertSallaerts
Copy link
Member

@GertSallaerts GertSallaerts commented Sep 24, 2018

This is a proposal of a new way to manage font-size on text-based avatars.

The current implementation makes the width of the text match avatarSize / textSizeRatio but this results in a lot of different font sizes. This happens because when you're not using a monospaced font, "JJ" for example will be significantly narrowed than "WW". This causes font-sizes to very even between avatars that all have two initials. (see image)

image

My proposal returns it back to the old implementation where font-size is set as avatarWidth / textSizeRatio. The problem with that implementation was that avatars with three of four initials could overflow the avatar's background. I propose to solve this by adding a new property textMarginRatio which reduces font-size only when the text would become to wide.

For now this is a POC; let me know what you think and I'll dot all the i's 😉

@JorgenEvens JorgenEvens changed the title improve font-size consitency with textMarginRatio property improve font-size consistency with textMarginRatio property Sep 24, 2018
@JorgenEvens
Copy link
Member

JorgenEvens commented Sep 24, 2018

The problem with the old implementation was that it would not behave as expected when using relative units (primarily percentages) for size. So I'm not really interested in returning back to the original font-size calculation.

Maybe we should try determining the fontSize based on the text height and make it fit within avatarWidth / textSizeRatio if larger than that. Or you know... use a monospace font by default.

I'd love to hear the opinion of @Sitebase and some of react-avatar's users before moving ahead.

The original issue is available at #136

@GertSallaerts
Copy link
Member Author

GertSallaerts commented Sep 24, 2018

I made a second commit that takes your first comment about not working with relatives sizes into account.


Maybe we should try determining the fontSize based on the text height and make it fit within avatarWidth / textSizeRatio if larger than that.

Your meaning is not clear to me so I can't comment any further on this.

@JorgenEvens
Copy link
Member

@GertSallaerts Could you finalise this PR as it seems to address most issues related to font-scaling.
I know I earlier didn't want to go with the old behaviour, but it seems that it used to be a lot more consistent for most users that seems to be the best way forward.

@GertSallaerts
Copy link
Member Author

Updated.

@JorgenEvens
Copy link
Member

Thanks, I'll look into merging this next week!

@JorgenEvens JorgenEvens merged commit 77b9d39 into ambassify:master Dec 16, 2018
JorgenEvens added a commit that referenced this pull request Dec 16, 2018
The font-scaling PR(#134) re-introduced bug (#133), this re-applies the fix
for that (#135)
@JorgenEvens JorgenEvens added this to the 3.5.0 milestone Dec 18, 2018
@JorgenEvens
Copy link
Member

This has been released as part of version v3.5.0!

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

Successfully merging this pull request may close these issues.

2 participants