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.

Sign up for GitHub

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

Jump to bottom

Types: Allow font to be partial scriptable and individually scriptable #10364

Merged
merged 8 commits into from May 25, 2022
Merged

Types: Allow font to be partial scriptable and individually scriptable #10364

merged 8 commits into from May 25, 2022

Conversation

LeeLenaleee
Copy link
Collaborator

Resolves #10363

@LeeLenaleee LeeLenaleee added the type: types Typescript type changes label May 19, 2022
@LeeLenaleee LeeLenaleee changed the title Types/scriptable font Types: Allow font to be partial scriptable and individually scriptable May 19, 2022
@LeeLenaleee
Copy link
Collaborator Author

Not really happy about the approach especially the CanvasFontSpec but didn't know an other/better way to achieve the desired behaviour

@etimberg etimberg requested a review from kurkle May 21, 2022 13:54
kurkle
kurkle reviewed May 21, 2022
View reviewed changes
Copy link
Member

@kurkle kurkle left a comment •
edited

Choose a reason for hiding this comment

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

I think the FontSpec should not include scriptability. Could create a ScriptableFontSpec or something if that is needed. The individual props of the font have not always been scriptable, but are they now?

@LeeLenaleee
Copy link
Collaborator Author

From the bit of testing I did it seemed like it. But will test it bit more when I get back from vacation in a few days.

etimberg
etimberg reviewed May 21, 2022
View reviewed changes
types/index.esm.d.ts Outdated Show resolved Hide resolved
@LeeLenaleee LeeLenaleee mentioned this pull request May 25, 2022
Open
LeeLenaleee
LeeLenaleee commented May 25, 2022
View reviewed changes
Copy link
Collaborator Author

@LeeLenaleee LeeLenaleee left a comment

Choose a reason for hiding this comment

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

Rename for consitency with ScriptableAndArray

types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
types/index.esm.d.ts Outdated Show resolved Hide resolved
kurkle
kurkle approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

lgtm

@etimberg etimberg merged commit 5173d05 into chartjs:master May 25, 2022
@etimberg etimberg added this to the Version 3.8.0 milestone May 25, 2022
@LeeLenaleee LeeLenaleee deleted the types/scriptable-font branch May 25, 2022 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@etimberg etimberg etimberg approved these changes

@kurkle kurkle kurkle approved these changes

@syedabdullahali syedabdullahali syedabdullahali left review comments

Assignees
No one assigned
Labels
type: types Typescript type changes
Projects
None yet
Milestone
Version 3.8.0
Development

Successfully merging this pull request may close these issues.

FontSpec returned by scripted font option requires all properties to be set, while they should be optional
4 participants
@LeeLenaleee @etimberg @kurkle @syedabdullahali