Skip to content

[IPPInAppFeedback] UI banner component design tweaks #8728

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
Merged
merged 16 commits into from
Jan 25, 2023
Merged

[IPPInAppFeedback] UI banner component design tweaks #8728

merged 16 commits into from
Jan 25, 2023

Conversation

@iamgabrielma
Copy link
Contributor

@iamgabrielma iamgabrielma commented Jan 23, 2023 •
edited
Loading

Part of #8530
Closes: #8674

Description

This PR updates the In-Person Payments Feedback banner UI design to make it closer to the design proposal.

Changes

  • Adds a new commentContent icon
  • Overrides the default .purple color for the Action links with .pink (confirmed with design at C0354HSNUJH/p1674112248357019-slack-C0354HSNUJH)
  • Appends a new top separator to the component by calling the existing createBorderView() in the main UIStackView

@toupper as you mentioned in the prior PR, the banner text doesn't go below the X icon for dismissal, but I haven't found a way to make this happen with the current implementation without having to modify it entirely. Happy to hear if you have any pointer or recommendation! 🙇

Testing instructions

On a store with COD enabled, and set to US or CA:

  1. Navigate to Orders, and see the In-Person Payments Feedback banner
  2. Confirm the icon, and color are correct as per the screenshot below:
Design proposal Current design
Screenshot 2023-01-23 at 15 30 37

@iamgabrielma
Add commentContent icon
d1779e0
@iamgabrielma
Change TopBannerView button color
2bc88d5 
While we’re moving towards making everything purple (pecCkj-fa-p2), some links are kept pink, so we can modify the component for all cases.
@wpmobilebot
Copy link
Collaborator

wpmobilebot commented Jan 23, 2023 •
edited
Loading

You can test the changes from this Pull Request by:
  • Clicking here or scanning the QR code below to access App Center
  • Then installing the build number pr8728-4579e44 on your iPhone

If you need access to App Center, please ask a maintainer to add you.

@iamgabrielma iamgabrielma added this to the 12.1 milestone Jan 23, 2023
@iamgabrielma iamgabrielma added type: enhancement A request for an enhancement. feature: mobile payments Related to mobile payments / card present payments / Woo Payments. labels Jan 23, 2023
@iamgabrielma iamgabrielma mentioned this pull request Jan 23, 2023
Closed
16 tasks
@iamgabrielma iamgabrielma requested a review from toupper January 23, 2023 07:37
toupper
toupper reviewed Jan 24, 2023
View reviewed changes
WooCommerce/Classes/Extensions/UIImage+Woo.swift Outdated
static var builtInReaderError: UIImage {
return UIImage(named: "built-in-reader-error")!
}

Copy link
Contributor

@toupper toupper Jan 24, 2023

Choose a reason for hiding this comment

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

Not a blocker, empty line here

Copy link
Contributor Author

@iamgabrielma iamgabrielma Jan 25, 2023

Choose a reason for hiding this comment

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

Fixed 3993a92

WooCommerce/Resources/Images.xcassets/icon-comment-content.imageset/Contents.json
@@ -0,0 +1,21 @@
{
"images" : [
{
Copy link
Contributor

@toupper toupper Jan 24, 2023

Choose a reason for hiding this comment

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

Perhaps you can apply here a single scale with the pdf file

Copy link
Contributor Author

@iamgabrielma iamgabrielma Jan 25, 2023

Choose a reason for hiding this comment

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

Right! Switched from SVG to PDF and universal size here: 26d3d06

@iamgabrielma
Copy link
Contributor Author

iamgabrielma commented Jan 25, 2023 •
edited
Loading

👋 Looping you in the PR as well @vanessamaartinstt . There's a known UI issue that we're discussing at p1674112248357019-slack-C0354HSNUJH :
Screenshot 2023-01-25 at 11 42 47

Edit: This is resolved.

@peril-woocommerce
Copy link

peril-woocommerce bot commented Jan 25, 2023

Warnings
⚠️ This PR is assigned to a milestone which is closing in less than 2 days Please, make sure to get it merged by then or assign it to a later expiring milestone

Generated by 🚫 dangerJS

@iamgabrielma iamgabrielma requested a review from toupper January 25, 2023 09:02
@iamgabrielma
Copy link
Contributor Author

iamgabrielma commented Jan 25, 2023

Resolved the issues, as well as the text moving below the dismiss button:

We were using an UILabel there, which I switched with a new StackView that center the label between two empty Views. I opted to add the same right space as the dismiss button size (24px), but can be increased further if needed.

Ready for re-review @toupper 🙇

@toupper toupper self-assigned this Jan 25, 2023
toupper
toupper reviewed Jan 25, 2023
View reviewed changes
WooCommerce/Classes/ViewRelated/Top Banner/TopBannerView.swift Outdated

func createLabelHolderStackView() -> UIStackView {
labelHolderStackView.addArrangedSubviews([
createSeparatorView(height: 48, width: 0),
Copy link
Contributor

@toupper toupper Jan 25, 2023

Choose a reason for hiding this comment

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

Could we add these values into constants? That way we can reuse them, and we know what they stand for

Copy link
Contributor Author

@iamgabrielma iamgabrielma Jan 25, 2023

Choose a reason for hiding this comment

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

Definitely! 4579e44

@toupper
Copy link
Contributor

toupper commented Jan 25, 2023

Great job @iamgabrielma ! Approved 🚢 :shipit:

@iamgabrielma iamgabrielma merged commit f752e43 into trunk Jan 25, 2023
@iamgabrielma iamgabrielma deleted the issue/8674-IPP-banner-UI branch January 25, 2023 10:28
@iamgabrielma iamgabrielma mentioned this pull request Jan 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toupper toupper toupper approved these changes

@vanessamaartinstt vanessamaartinstt Awaiting requested review from vanessamaartinstt

Assignees

@toupper toupper

Labels

feature: mobile payments Related to mobile payments / card present payments / Woo Payments. type: enhancement A request for an enhancement.

Projects

None yet

Milestone

12.1

Development

Successfully merging this pull request may close these issues.

[IPPInAppFeedback] Polish UI banner component design

4 participants

@iamgabrielma @wpmobilebot @toupper