-
Notifications
You must be signed in to change notification settings - Fork 121
[IPPInAppFeedback] UI: Payments banner placement #8532
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
Conversation
You can test the changes from this Pull Request by:
|
...s/ViewRelated/Dashboard/Settings/In-Person Payments/InPersonPaymentsMenuViewController.swift
Outdated
Show resolved
Hide resolved
...s/ViewRelated/Dashboard/Settings/In-Person Payments/InPersonPaymentsMenuViewController.swift
Outdated
Show resolved
Hide resolved
|
Thanks for opening this PR @iamgabrielma. I added a couple of comments, and since testing might change depending on how the comments are addressed, I will wait for that before testing. |
...s/ViewRelated/Dashboard/Settings/In-Person Payments/InPersonPaymentsMenuViewController.swift
Outdated
Show resolved
Hide resolved
...s/ViewRelated/Dashboard/Settings/In-Person Payments/InPersonPaymentsMenuViewController.swift
Outdated
Show resolved
Hide resolved
| /// | ||
| #if DEBUG | ||
| case inAppFeedback = "https://automattic.survey.fm/woo-app-general-feedback-test-survey" | ||
| case IPPinAppFeedback = "https://automattic.survey.fm/woo-app-ipp-in-app-feedback-testing" |
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.
Now that we have two inApp feedbacks, the naming looks a bit confusing to me. To make it clearer I would simply name it
generalFeedback and
IPPFeedback
We can omit the inApp prefix, we know that the feedback is triggered from the app.
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.
|
Thanks for the review @toupper ! Due to design changes on this feature I've redone the banner to use a
If this seems a good approach, I'll polish the UI further, as I may need to create a new Notice component that looks closer to what we need. I found other components in the codebase that may be helpful, like
Yes, that was a mistake on my end when pushing the branch 😅 , the rest of issues are labeled and tracked in a better way. |
Generated by 🚫 dangerJS |
|
Closing this issue due to modifications to scope ( pdfdoF-20T-p2 ), as we won't be showing this banner anymore in the Payments view. |


Part of #8530
Description
This PR adds the in-app feedback banner in the Payments view, as well as the buttons to launch a survey and dismiss the banner.
At the moment the survey is only dismissible when tapping on "dismiss" (not upon survey completion), and will show up again each time we navigate to the Payments view.
TODO:
FeatureAnnouncementCardViewChanges
Noticecomponent to create a feedback notice banner in the Payments View.noticeTappedHandlerhandler noticeTappedHandler, so we can use 2 different behaviours: When the notice is tapped (show survey), and when the action button is tapped (dismiss notice).IPPFeedbackcase toSurveyViewControllerwhich will launch and manage the Survey launched via the tapping the noticehttps://automattic.survey.fm/woo-app-ipp-in-app-feedback-testing), this will dynamically change in future PRs based on certain conditions.Testing instructions
Screenshots