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 options to style passed in BitmapText constructor. #6671

Merged
merged 11 commits into from
May 31, 2020

Conversation

ShukantPal
Copy link
Member

@ShukantPal ShukantPal commented May 28, 2020

#6667

Description of change
  • style.tint passed to BitmapText will now update the _tint property of the BitmapText and not this._font.

  • Furthermore, the _tint field from _font is removed (it was never referenced anywhere else).

  • Added additional options to IBitmapTextStyle: letterSpacing, maxWidth, maxLineHeight, anchor, roundPixels

  • Updated the BitmapText constructor parameter documentation to reflect the above options

Pre-Merge Checklist
  • Documentation is changed or added
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@codecov-commenter
Copy link

codecov-commenter commented May 28, 2020

Codecov Report

Merging #6671 into dev will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               dev     #6671   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines          678       678           
=========================================
  Hits           678       678           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 89305c5...6b7f8bd. Read the comment docs.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Thanks for taking a stab at this. I'm really not crazy about this existing _font object. What if we deprecate that font getter/setter and break out the properties separately. Why does this have to be an object? Can't we just have separate properties for name/size/align?

Also, we should make sure that font is deprecated properly for backward compatibility support.

packages/text-bitmap/src/BitmapText.ts Outdated Show resolved Hide resolved
packages/text-bitmap/src/BitmapText.ts Outdated Show resolved Hide resolved
ShukantPal and others added 7 commits May 28, 2020 16:26
Co-authored-by: Matt Karl <matt@mattkarl.com>
All subproperties of _font are lifted up to BitmapText properties.

Signed-off-by: Shukant Pal <shukantpal@outlook.com>
Signed-off-by: Shukant Pal <shukantpal@outlook.com>
Signed-off-by: Shukant Pal <shukantpal@outlook.com>
Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

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

Okay, I'm good with this one. I made a couple of tweaks:

  • Added setters for fontName and fontSize
  • Fixed the deprecation (both in constructor and font setter)
  • Defined defaults using Object.assign
  • Upgrade deprecations in tests
  • Removes roundPixels and anchor from style options (these are rendering options, not "style")

Here's a demo just to prove that the deprecations still work:
https://jsfiddle.net/bigtimebuddy/har5kmcf/

const text = new PIXI.BitmapText(
  'Hello World',
  { font: fontName } // deprecated, must be `fontName` as style prop
);
text.font = fontName; // setter has ben removed
text.font = '20px ' + fontName; // setter with size has been removed
text.font = { name: fontName, size: 20 }; // set as an object has been removed

@ShukantPal ShukantPal requested a review from a team May 29, 2020 16:27
Copy link
Member

@GoodBoyDigital GoodBoyDigital left a comment

Choose a reason for hiding this comment

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

👍

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label May 31, 2020
@bigtimebuddy bigtimebuddy merged commit b66d864 into dev May 31, 2020
@bigtimebuddy bigtimebuddy deleted the fix/bitmap-text-style branch May 31, 2020 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants