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

[MBL-970] Report Project Info View #1852

Merged
merged 23 commits into from Sep 23, 2023
Merged

[MBL-970] Report Project Info View #1852

merged 23 commits into from Sep 23, 2023

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Sep 18, 2023

📲 What

Implement a “Report this project to Kickstarter” button at the bottom of each project page. Clicking this button will open an info view describing the process for reporting projects and when we will/won’t take action displayed in an expandable list.

This view will also include links to a form to report problematic content.

We don't have official designs for this so we're following android's implementation as an example.

🤔 Why

Our iOS app currently violates the User Generated Content Policy in the App Store. Specifically, we need (at the project screen level):

  • A method for filtering objectionable content
  • A mechanism for users to flag objectionable content
  • A mechanism for users to block abusive users

🛠 How

  • Adds a new "Report this project to Kickstarter" label at the bottom of the overview section of Project pages in ProjectPamphletSubpageCellViewModel (responsible for displaying comments and updates currently)
  • When tapping this new label, a new SwiftUI View ReportProjectInfoView is pushed onto the nav stack displaying our reporting info in an expandable list.
    • Some text includes hyperlinks so, I've added a couple helpers to make it easier to parse the html strings that we need to use here.
    • Tapping on an expanded row takes you to a blank view. This will be the submit report form that will be built in the next PR. ticket here

👀 See

Simulator Screen Recording - iPhone 14 - 2023-09-20 at 16 31 32

TODO

  • I will wrap this behind the feature flag in the next PR

* displays different reporting categories in a nested dropdown style list
* some text needs to contain hyperlinks so I added helpers to make this easier.
* displays different reporting categories in a nested dropdown style list
* some text needs to contain hyperlinks so I added helpers to make this easier.
@scottkicks scottkicks self-assigned this Sep 18, 2023
@scottkicks scottkicks added this to the release-5.11.0 milestone Sep 18, 2023
@@ -99,6 +102,9 @@ public protocol ProjectPageViewModelOutputs {
/// Emits a `Project` when the updates are to be rendered.
var goToUpdates: Signal<Project, Never> { get }

/// Emits a `Project` when the report project view is to be rendered.
var goToReportProject: Signal<Project, Never> { get }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to pass a Project here because the submission form will need to include the project url. https://kickstarter.slack.com/files/U0558SFCTL5/F05SJHQP23A/screenshot_20230911-125644.png

Copy link
Contributor

Choose a reason for hiding this comment

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

So just pass in the project url?

type: .parent,
title: Strings.Report_spam(),
subtitle: Strings
.Our(community_guidelines: "https://www.kickstarter.com/help/community?ref=project-page-report"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we have these URLs somewhere in the app already? I wasn't seeing them and I'd rather not hard code these if we can avoid it.

@scottkicks scottkicks marked this pull request as ready for review September 18, 2023 20:30
@@ -44,15 +44,18 @@ public final class ProjectPamphletSubpageCellViewModel: ProjectPamphletSubpageCe
public init() {
let commentsSubpage = self.subpageProperty.signal.skipNil().filter { $0.isComments }
let updatesSubpage = self.subpageProperty.signal.skipNil().filter { $0.isUpdates }
let reportProjectSubpage = self.subpageProperty.signal.skipNil().filter { $0.isReportProject }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking now that I might want to refactor in the next PR where we'll add the "You've already reported this project" label. Rather than add them as a cell to this Pamphlet Subpage Cell, we could add as there own container below it in the tableview. Making it easier to swap between the two and keep this feature entirely SwiftUI.

Copy link
Contributor

@msadoon msadoon Sep 20, 2023

Choose a reason for hiding this comment

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

Think about refactoring this entire cell - entirely in SwiftUI. It looks pretty simple (label - label) setup.
That way we can remove ProjectPamphletSubpageCell entirely. If time constrained no need to do it in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this is out of scope for this PR

@kickstarter kickstarter deleted a comment from nativeksr Sep 20, 2023
@kickstarter kickstarter deleted a comment from codecov bot Sep 20, 2023
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #1852 (bcdf584) into main (e0657bf) will decrease coverage by 0.08%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##             main    #1852      +/-   ##
==========================================
- Coverage   84.06%   83.99%   -0.08%     
==========================================
  Files        1280     1283       +3     
  Lines      116823   116986     +163     
  Branches    31043    31111      +68     
==========================================
+ Hits        98207    98258      +51     
- Misses      17532    17641     +109     
- Partials     1084     1087       +3     
Files Changed Coverage Δ
...Features/ReportProject/ReportProjectInfoView.swift 0.00% <0.00%> (ø)
KsApi/models/ReportProjectInfoListItem.swift 0.00% <0.00%> (ø)
Library/String+Attributed.swift 65.62% <0.00%> (-34.38%) ⬇️
Library/SwiftUI+Extensions/Text+HTML.swift 0.00% <0.00%> (ø)
...ectPage/Controller/ProjectPageViewController.swift 50.07% <16.66%> (-0.64%) ⬇️
...ewModels/ProjectPamphletSubpageCellViewModel.swift 79.10% <66.66%> (-4.54%) ⬇️
Library/HelpType.swift 87.67% <75.00%> (-0.74%) ⬇️
...tasource/ProjectPageViewControllerDataSource.swift 83.89% <100.00%> (+0.13%) ⬆️
...rce/ProjectPageViewControllerDataSourceTests.swift 97.46% <100.00%> (+0.05%) ⬆️
Library/ViewModels/ProjectPageViewModel.swift 97.31% <100.00%> (+0.04%) ⬆️
... and 1 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

Good work, I found some things worth addressing but most of the essentials are here.

I found a few notable issues wrt to how this looks and feels:

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-09-20.at.11.19.31.mp4
  1. Mentioned below but the navigation title and back button should be in one navigation bar. See "Updates" and "Comments" for a consistent user experience.
  2. I noticed in the playground as well, the list view is janky, especially loading that first list of items. I think it might be the way we render the .child objects. Look into fixing it because it is pretty obvious.
  3. Also - you're probably aware but the double back button on the "submit report view" page should be fixed. I think with the 1) fix above, you can get this one too.
  4. Minor - disclosure indicator beside the subpage cell text like in the designs.

Not related to the video
Screenshot 2023-09-20 at 11 33 02 AM

  • noticed some background color border issues running on iOS 17 for the count values beside "Comments" and "Updates".

self.viewModel.inputs.showNavigationBar(false)
self.navigationController?
.pushViewController(UIHostingController(rootView: reportProjectInfoView), animated: true)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we'll be releasing this with the iOS 17 upgrade?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is blocking our next release and needs to go out in 5.11.0.

private func goToReportProject(project _: Project) {
if #available(iOS 15, *) {
let reportProjectInfoView = ReportProjectInfoView()
self.viewModel.inputs.showNavigationBar(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be tied to the weird navigation bar behaviour in the review overview video.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you look at goToComments and goToUpdates you'll see that we tell the viewmodel to hide the nav bar before pushing those onto the stack. I think this is because the project page is being presented as a sheet. If we don't manually call self.viewModel.inputs.showNavigationBar(false) then the project page navbar (has the x, share, and heart icons) remains on pushed views.

I'll work on getting the new view's nav title to be inline at the top, but I think this is how we need to do this for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Designs show a disclosure indicator to indicate tappable cell. I don't see one here beside "Report this project to Kickstarter"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've decided to refactor this label to be it's own swiftUI view to make it easier to toggle between this and the "you've already reported" label in my next PR (MBL-983).

I'll be sure to include a disclosure indicator in that.

@@ -44,15 +44,18 @@ public final class ProjectPamphletSubpageCellViewModel: ProjectPamphletSubpageCe
public init() {
let commentsSubpage = self.subpageProperty.signal.skipNil().filter { $0.isComments }
let updatesSubpage = self.subpageProperty.signal.skipNil().filter { $0.isUpdates }
let reportProjectSubpage = self.subpageProperty.signal.skipNil().filter { $0.isReportProject }
Copy link
Contributor

@msadoon msadoon Sep 20, 2023

Choose a reason for hiding this comment

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

Think about refactoring this entire cell - entirely in SwiftUI. It looks pretty simple (label - label) setup.
That way we can remove ProjectPamphletSubpageCell entirely. If time constrained no need to do it in this PR.

var body: some View {
if item.type == .child {
// TODO: Push Submission Form View In MBL-971(https://kickstarter.atlassian.net/browse/MBL-971)
NavigationLink(destination: { Text("submit report view") }, label: { BaseRowView(item: item) })
Copy link
Contributor

Choose a reason for hiding this comment

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

Internationlized text for this?

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 is just a placeholder for the new view that will actually be pushed

type: .parent,
title: Strings.Report_spam(),
subtitle: Strings
.Our(community_guidelines: "https://www.kickstarter.com/help/community?ref=project-page-report"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Try to use existing methods of placing urls like HelpType url(withBaseUrl

Copy link
Contributor

Choose a reason for hiding this comment

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

Recent example use case was the ProjectTabDisclaimerCell

* List view has a bug in iOS 16 that causes expandable rows to have poor animations. My solution uses a ScrollView and a manual Expanded property instead
* Also addresses the double navigation bar issues
@scottkicks
Copy link
Contributor Author

scottkicks commented Sep 20, 2023

@msadoon All of your callouts have been addressed.

re: the janky list view animations. It looks like a bug was introduced in iOS 16.4 that I don't think I can get around by using a List. Instead, I was able to use a ScrollView and a Set object to manually animate a row item's sub views.
It ended up being a pretty simple solution and you'll see that everything animates much more smoothly now!

I also fixed the double navigation bar issue. Since we're using a NavigationController in UIKit to push the view we don't actually need to wrap our SwiftUI view in a NavigationView. We get it for free!

I believe everything else has been covered.

I will re-request your review once CI passes.

Copy link
Contributor

@ifosli ifosli left a comment

Choose a reason for hiding this comment

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

Overall, I think this looks good. A couple things:

  • voiceover doesn't work at all. I'm not sure what's going on (haven't done anything with voiceover and swiftui before) but we need to fix that before submitting. I'd be happy to investigate and aim to fix that in a separate pr.

  • I don't agree with the arrow directions. Arrow down should indicate something that expands down and arrow up should indicate something that can collapse back up. Arrow right should indicate new window. But this is maybe just my opinion and not that important, so I'm okay with the way it is if you like it better and it matches android.

@available(iOS 15, *)
init(html: String, with hyperlink: String) {
do {
var attrString = try html.htmlToAttributedString()
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with Mubarak here - as we gradually update our app to swiftui, I'd personally prefer not to move files around while we do so.

@scottkicks
Copy link
Contributor Author

@ifosli I forgot to test voiceover. It would definitely be helpful if you took a look at that.

Feeling a bit rushed with this since code freeze is next week. We still need to build the view that allows users to submit reports as well as the "You've already reported" label (working in that now). Anything you can pick up would be helpful.

As for the arrow icons, I see your point. I was trying to follow androids design as closely as possible. Without official designs tho, I think it's something we can address after the release is unblocked. Thoughts?

@scottkicks scottkicks merged commit ae5ef71 into main Sep 23, 2023
7 checks passed
@scottkicks scottkicks deleted the report-this-project-views branch September 23, 2023 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants