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

[experiment] add 4px horizontal padding to text #2855

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

steveruizok
Copy link
Collaborator

This PR adds padding to the left and right of text, similar to arrow labels.

Kapture 2024-02-16 at 20 41 03

Change Type

  • patch — Bug fix

Test Plan

  1. Check out the delta on large projects

@huppy-bot huppy-bot bot added the bugfix Fixes a bug of some kind label Feb 16, 2024
Copy link

vercel bot commented Feb 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
examples ✅ Ready (Inspect) Visit Preview Feb 16, 2024 8:43pm
1 Ignored Deployment
Name Status Preview Updated (UTC)
tldraw-docs ⬜️ Ignored (Inspect) Feb 16, 2024 8:43pm

@steveruizok
Copy link
Collaborator Author

I considered adding a -4px offset to all text shapes, however this might disrupt alignment in projects; and disrupting alignment may actually be a bigger change than the visual offset.

@SomeHats
Copy link
Contributor

I feel like the thing we're trying to do here is make the top/bottom spacing (which is sort of a function of the font and line-height) be about the same as the left-right spacing. So I think we should make this be proportional to the font size, rather than a hard-coded 4px. It avoids quirks like this:
Screenshot 2024-02-19 at 11 21 28
Screenshot 2024-02-19 at 11 24 16

Here, we have two different sizes of text (S & XL) scaled to be the same size (text sizing lol). Both of these look a bit off - the padding is too big on the top (S) and too small on the bottom (XL).

Also, curious about the alignment thing! What do you mean by that?

@mimecuvalo
Copy link
Contributor

drive-by comment: i'd love for us to have a cssConstants.ts file that auto-generates var(--padding-for-something) for use in a .css file so that the same constant can be use across the .ts/.css chasm. happy to help build that separately for this kind of thing that will get out-of-sync potentially.

@steveruizok
Copy link
Collaborator Author

Here, we have two different sizes of text (S & XL) scaled to be the same size (text sizing lol). Both of these look a bit off - the padding is too big on the top (S) and too small on the bottom (XL).

Oh I'd overlooked scaling. At minimum we'd need to divide the padding by scale.

@steveruizok steveruizok changed the title [substantial tweak] add 4px horizontal padding to text [experiment] add 4px horizontal padding to text Mar 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug of some kind
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants