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

Updated with PayPalOpen font #237

Merged
merged 22 commits into from
Jan 24, 2024
Merged

Updated with PayPalOpen font #237

merged 22 commits into from
Jan 24, 2024

Conversation

stechiu
Copy link
Contributor

@stechiu stechiu commented Jan 8, 2024

Reason for changes

Need to update fonts for Payment Buttons to PayPalOpen font as the primary font as part of QL.

Figma PR
Screenshot 2024-01-08 at 12 01 09 PM Screenshot 2024-01-08 at 12 01 30 PM

Summary of changes

  • Added new fonts
  • Replaced Payment fonts with new fonts

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

@stechiu

@stechiu stechiu requested a review from a team as a code owner January 8, 2024 20:03
@stechiu
Copy link
Contributor Author

stechiu commented Jan 8, 2024

Since the change is in the font typeface and isn't breaking, nor do Merchants need to make any changes, does the change still need to be included in the CHANGELOG?

@scannillo
Copy link
Collaborator

scannillo commented Jan 8, 2024

Since the change is in the font typeface and isn't breaking

I would classify changing UI without a merchant having to opt-in to that change, as a breaking change. Even though it's super minor (in this case of a font that doesn't look too different), by the book it would be breaking.

We could classify this is a "bug-fix to meet brand guidelines" if we want to get it into V1 😆. Sketchy? Thoughts?

@jaxdesmarais
Copy link
Collaborator

We could classify this is a "bug-fix to meet brand guidelines" if we want to get it into V1 😆. Sketchy? Thoughts?

I think classifying this as a bug fix makes sense! I assume there is some "brand reputation" to actually using the correct fonts so could make sense for a minor version + changelog update.

@stechiu
Copy link
Contributor Author

stechiu commented Jan 9, 2024

Since the change is in the font typeface and isn't breaking

I would classify changing UI without a merchant having to opt-in to that change, as a breaking change. Even though it's super minor (in this case of a font that doesn't look too different), by the book it would be breaking.

We could classify this is a "bug-fix to meet brand guidelines" if we want to get it into V1 😆. Sketchy? Thoughts?

Got it, that makes sense! I read through the Semantic Versioning Guideline and must've overlooked UI related changes. In that case, we can have it go into V2 where we have other breaking changes instead.

@scannillo
Copy link
Collaborator

must've overlooked UI related changes

You definitely didn't! UI specific stuff isn't clearly defined in there.

I like Jax's thoughts - I'm also down for lumping into V1!

Comment on lines 20 to 22
case .mini, .collapsed, .expanded, .full:
return UIFont(name: "PayPalOpen-Regular", size: 14) ??
.systemFont(ofSize: UIFont.systemFontSize)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI: Button sizes will reduce to .mini and .expanded in another PR

Demo/Demo/Info.plist Outdated Show resolved Hide resolved
Demo/Demo/Info.plist Outdated Show resolved Hide resolved
* Added regular font file back to Demo app
CHANGELOG.md Outdated Show resolved Hide resolved
Sources/PaymentButtons/PaymentButtonSize.swift Outdated Show resolved Hide resolved
@@ -2,6 +2,10 @@
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>UIAppFonts</key>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have a ticket to add documentation for this for merchants? It seems like this is something that a merchant would need to do to use our fonts. What happens if they don't add this to their Info.plist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line was added in the CHANGELOG under Payment Buttons so that merchants know about the font change: Font typeface changed to "PayPalOpen" to meet brand guidelines

The font shouldn't be in Info.plist so I've removed it. The font is registered in the SDK under the PaymentButtonFont file so that merchants won't need to add the font themselves

Copy link
Contributor

@richherrera richherrera left a comment

Choose a reason for hiding this comment

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

🚀

Co-authored-by: Rich Herrera <[email protected]>
@stechiu stechiu merged commit 6c25b9f into main Jan 24, 2024
4 checks passed
@stechiu stechiu deleted the button-typeface-update branch January 24, 2024 21:22
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.

6 participants