-
Notifications
You must be signed in to change notification settings - Fork 110
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
[Woo POS] Design iteration on Simple Products banner #13940
[Woo POS] Design iteration on Simple Products banner #13940
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Excellent! Thanks for a helpful description with images.
I left one comment for the font of Learn more
text.
private var bannerHintAndLearnMoreText: Text { | ||
Text(Localization.headerBannerHint + " ") + | ||
Text(Localization.headerBannerLearnMoreHint) | ||
.font(.body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the text and learn more button should be posDetailEmphasized
.font(.body) | |
.font(.posDetailEmphasized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! 7acf98a , interestingly we have to declare the type and cast to font()
or the types don't match and the compiler is unhappy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, interesting. I played around and something like this worked. Likely related to the fact that we're appending two Texts here.
private var bannerHintAndLearnMoreText: some View {
(Text(Localization.headerBannerHint + " ") +
Text(Localization.headerBannerLearnMoreHint))
.font(.posDetailEmphasized)
.foregroundColor(Color(.accent))
}
@@ -109,6 +108,13 @@ private extension ItemListView { | |||
.padding(.horizontal, Constants.bannerCardPadding) | |||
} | |||
|
|||
private var bannerHintAndLearnMoreText: Text { | |||
Text(Localization.headerBannerHint + " ") + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's crazy how easy it is just to append two Text
objects on SwiftUI
. Much harder to archieve the same effect on UIKit
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't looking forward to attributed strings, definitely an improvement :D
Closes: #13939
Description
This PR updates the Simple Products banner following the design feedback on #13859 and TfaZ4LUkEwEGrxfnEFzvJj-fi-4650_22247
Changes
Learn More
to be interpolated within the rest of the text, rather than appearing on a separate line. Also adjusted the height between lines.Testing
ItemListViewModel
init:RELEASE-NOTES.txt
if necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: