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 Black Boundary on White Button #273

Merged
merged 5 commits into from
May 23, 2024
Merged

Conversation

KunJeongPark
Copy link
Collaborator

@KunJeongPark KunJeongPark commented May 20, 2024

Reason for changes

Inbound from Merchant LI-47312 asking for parity on appearance of white button having
black boundary on Android SDK vs none in iOS SDK (no black boundary on white button)

Summary of changes

  • Add black boundary if chosen color is white.

Before:
buttonInboundPic

After:
Pay Later Button
PayPal Button
PayPal Button Mini

Checklist

  • Added a changelog entry

Authors

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

@KunJeongPark KunJeongPark requested a review from a team as a code owner May 20, 2024 18:33
Comment on lines 337 to 343
if self.color == .white {
self.containerView.layer.borderColor = UIColor.black.cgColor
self.containerView.layer.borderWidth = 2
} else {
self.containerView.layer.borderColor = UIColor.clear.cgColor
self.containerView.layer.borderWidth = 0
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if self.color == .white {
self.containerView.layer.borderColor = UIColor.black.cgColor
self.containerView.layer.borderWidth = 2
} else {
self.containerView.layer.borderColor = UIColor.clear.cgColor
self.containerView.layer.borderWidth = 0
}
containerView.layer.borderColor = color == .white ? UIColor.black.cgColor : UIColor.clear.cgColor
containerView.layer.borderWidth = color == .white ? 2 : 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed here: f55c9fe

Copy link
Collaborator

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

How does Android handle the boarder in dark mode? It seems like it'd have the same issue as a white button. Not sure if we want to consider triple checking with a designer on how/if the boarder on the white button should be handled for light vs dark mode.

@KunJeongPark
Copy link
Collaborator Author

KunJeongPark commented May 23, 2024

How does Android handle the boarder in dark mode? It seems like it'd have the same issue as a white button. Not sure if we want to consider triple checking with a designer on how/if the boarder on the white button should be handled for light vs dark mode.

I looked at Android code and they have logic in there where if color is white, black border with 2 points is made.
Nirvan made a decision to stay consistent with JS and Android and have black border around white button. Merchant wanted parity between Android and iOS either way.

I will post design docs on Slack, looks like there was black boundary around it.

Let me check how it looks on dark mode, that's a good point.
It looks same in dark mode. ✅
I think as long as system colors are not used, there is no automatic handling of color scheme for light/dark modes.
Android also has light and dark modes, I believe, but there is no separate handling of this for the black border on white buttons.

I guess issue would be visibility around white button against white or off white background colors if there is no border.

Since there is no separate treatment on the button colors on dark/light themes, I think that should be a separate ticket.
We did make a decision on parity between Android and iOS.

I can check on devices, check if their look is consistent across different modes and system background colors.


private func customizeAppearance() {
containerView.layer.borderColor = color == .white ? UIColor.black.cgColor : UIColor.clear.cgColor
containerView.layer.borderWidth = color == .white ? 2 : 0
Copy link
Contributor

Choose a reason for hiding this comment

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

What does Android use for border width? Looks like it's 1px but I'm not sure because the Merchant's image of the buttons you shared in Slack is very small. Figma designs show 1px https://www.figma.com/design/gn4AXdmJUOMVlrCMpyhfrf/QL---Checkout-Buttons?node-id=1-73&t=C9Kq3Q5LMzl7DqsD-0

Copy link
Collaborator Author

@KunJeongPark KunJeongPark May 23, 2024

Choose a reason for hiding this comment

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

Ah thank you! I saw 2 in Android. But it did look a bit thick to me on screenshot for iOS.

It was 1 point in Android defined as a constant. Will make the change.

Copy link
Collaborator

@jaxdesmarais jaxdesmarais 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 following up on my questions! 🚀

@KunJeongPark KunJeongPark merged commit cc01ee2 into main May 23, 2024
5 checks passed
@KunJeongPark KunJeongPark deleted the white-button-boundary branch May 23, 2024 19:34
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