Skip to content

Conversation

@TheAlexLichter
Copy link

According to the docs, fontSize is an optional property. This should be reflected in the types as well.

@github-actions
Copy link

github-actions bot commented Mar 20, 2025

CLA Assistant Lite bot ✅ All required contributors have signed the F5 CLA for this PR. Thank you!

@TheAlexLichter
Copy link
Author

I have hereby read the F5 CLA and agree to its terms

@lee00678
Copy link
Collaborator

@TheAlexLichter thanks for the fix!

@rokotyan
Copy link
Contributor

@TheAlexLichter (@lee00678)

I'm not confident that this property is optional, how did you come to your conclusion?

Looking at the getWrappedText function, the fontSize property doesn't seem to be optional
image

@lee00678
Copy link
Collaborator

lee00678 commented Mar 21, 2025

@TheAlexLichter (@lee00678)

I'm not confident that this property is optional, how did you come to your conclusion?

Looking at the getWrappedText function, the fontSize property doesn't seem to be optional.

I haven't got a chance to review this PR. But I'm also open to make it an optional property.

@TheAlexLichter
Copy link
Author

TheAlexLichter commented Mar 21, 2025

@rokotyan Given this example in the docs (and others) not adding explicit fontSize values.

### Styled Text
To customize font or color of your text, provide `content` with an object of type `UnovisText`.
In this case, you put the desired string content to the `text` property,
and additional properties can be set.
```ts
items = [{
content: { text: 'Item 2', color: 'red' }
}]
```
<XYWrapper {...annotationsProps()} excludeTabs showAxes items={[
{ content: { text: 'Item 2', color: 'red' } }
]}/>

And that there are defaults in

@rokotyan
Copy link
Contributor

@TheAlexLichter I see. It's optional only for the Annotations component, so the type should be this:

image

@rokotyan
Copy link
Contributor

Thanks for bringing up this inconsistency!

@TheAlexLichter
Copy link
Author

@rokotyan Good point! Updated my PR to reflect that! ☺️

@rokotyan
Copy link
Contributor

Thanks!

@lee00678
Copy link
Collaborator

@TheAlexLichter could you please take a look at this: https://github.com/f5/unovis/actions/runs/14013488915/job/39393838730?pr=542? Thanks

@rokotyan
Copy link
Contributor

I've checked this again, and found that merging with default happens here. So the original idea of making fontSize of UnovisText optional was probably the way to go. We just need to make sure that other functions that work with UnovisText don't rely on the provided fontSize.

Maybe it's worth renaming UnovisText to UnovisTextPartial, and adding a new UnovisText type with all the properties marked as mandatory.

@lee00678
Copy link
Collaborator

Fixed!

@lee00678 lee00678 closed this Jun 27, 2025
@TheAlexLichter TheAlexLichter deleted the fix/font-size branch June 28, 2025 20: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.

3 participants