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

feat: additionalElementStyles prop on fitText and measureText #3685

Merged
merged 17 commits into from Apr 9, 2024

Conversation

Just-Moh-it
Copy link
Contributor

With all the existing inputs fitText and measureText expose, it's not possible to use these functions for more complex inputs where you need more control over the underlying <span> element that gets created to measure the text

Example

Before the addition

Adding a property like this:

Screenshot 2024-04-08 at 11 15 35 PM

Would make the fitText and measureText break and wrap the output.

image

After the addition,

Screenshot 2024-04-08 at 11 17 04 PM

It properly fits the text in now!

Screenshot 2024-04-08 at 11 18 25 PM

Why not just expose textTransform in as an input instead of creating a totally new additionalElementStyles

I use this function in a project that uses this function in a complex text element inside a editor that supports various font properties on an element, like textTransform, transform, etc., which I don't think would make sense to have as explicit inputs to this function.

As for the risk of specifying fontFamily or fontSize, or any other property through additionalElementStyles, I've excluded them from the type for this property, hence leaving no scope of accidentally specifying the same property twice, once through the direct function params, and again through additionalElementStyles.

What could be improved

  • I think the name additionalElementStyles is not the best and could be better. It says element in the name, whereas the docs don't specify what the element actually is. Unless someone digs through the source code and finds out the measureText function actually creates an invisible text element in the DOM, they won't know this abstraction exists. Feels like it exposes parts of the abstraction that haven't been documented.
  • Naturally, the second would be 'how it works' section on fitText docs, which says something like "it uses measureText under the hood to calculate the size of the text with a standard fontSize, then uses the ratio of the resulting element's width to proportionally find the fontSize that would fit into the passed in withinWidth property. This property exposes the styles on the underlying invisible span element that is created", and something similar on measureText as well.

Copy link

vercel bot commented Apr 9, 2024

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

Name Status Preview Comments Updated (UTC)
bugs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 9, 2024 7:46am
remotion ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 9, 2024 7:46am

@JonnyBurger
Copy link
Member

Thanks a lot for the PR! 🙌🏼 💙
Sounds good to me.

I changed it to additionalStyles for brevity.
And added support for textTransform as a top-level option!

@JonnyBurger JonnyBurger merged commit bd98a39 into remotion-dev:main Apr 9, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants