Skip to content

Conversation

M-i-k-e-l
Copy link
Collaborator

Description

DynamicFonts - add isFontFamilyDownloaded

Changelog

None

Additional info

None

@M-i-k-e-l M-i-k-e-l added the Important for Next Release PR that must be included in the release version label Oct 3, 2023
@M-i-k-e-l M-i-k-e-l added this to the Design Kits milestone Oct 3, 2023
Copy link
Collaborator

@ethanshar ethanshar left a comment

Choose a reason for hiding this comment

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

Approved.
But if I understand the code correctly, I'd rename this method, or maybe unify it with the method above

@@ -173,4 +173,23 @@ export default class DynamicFonts {
public async isFontDownloaded(fontName: string, fontExtension: FontExtension): Promise<boolean> {
return await this.fontDownloader.isFontDownloaded(fontName, fontExtension);
}

public async isFontFamilyDownloaded(rootUri: string,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It sounds like you named the method with the purpose you need it for while technically this method checks if multiple fonts are downloaded, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It checks if the whole font family is downloaded, yes; all the methods in this API are this way.
We can try to think of a different API if you like, but I'm merging for now.

@M-i-k-e-l M-i-k-e-l merged commit 8d7cb5c into master Oct 4, 2023
@M-i-k-e-l M-i-k-e-l deleted the feat/dynamic-fonts-add-is-font-family-downloaded branch October 4, 2023 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Important for Next Release PR that must be included in the release version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants