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

Add font size argument in text Wireframe builder #1613

Merged

Conversation

louiszawadzki
Copy link
Contributor

What and why?

Add font size argument in createTextWireframe public builder.

Today passing a font argument is the only way to specify a font size for the text wireframe.
With React Native text views we don't get direct access to the UIFont. However we get direct access to the fontSize,
so we can bypass this by creating a new font on every call, but that takes a lot of time and impacts performance (on an app I tested, it took up to 300ms of time over a minute of usage of the app).

I've tried passing nil as font (DataDog/dd-sdk-reactnative#570), but then all elements have a small font size.

I believe enabling to specify only the font size is the best way to get a great fidelity with a small performance cost.

How?

It seems that there is no test covering this part of the code, let me know if I should add some.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@louiszawadzki louiszawadzki requested review from a team as code owners January 4, 2024 17:01
Comment on lines 100 to 101
font: UIFont? = nil,
fontSize: CGFloat? = nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't entirely like the fact that it's now implicitly either fontSize or font.size. Maybe we could rename to fontSizeOverride ?

maciejburda
maciejburda previously approved these changes Jan 4, 2024
Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One non-blocking suggestion

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented Jan 4, 2024

Datadog Report

Branch report: louiszawadzki/add-font-size-in-text-wireframe
Commit report: bea5879
Test service: dd-sdk-ios

✅ 0 Failed, 256 Passed, 0 Skipped, 4m 35.27s Wall Time

@louiszawadzki louiszawadzki merged commit c91484f into develop Jan 5, 2024
4 checks passed
@louiszawadzki louiszawadzki deleted the louiszawadzki/add-font-size-in-text-wireframe branch January 5, 2024 09:26
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.

2 participants