-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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-983] Already Reported Project Label #1857
Conversation
* UITableView cell will be added to the Project Page Table View * Used to display a new swiftui view for the report this project label and already reported label
ReportProjectLabelView (SwiftUI View) re-renders when its text labels contain hyperlinks. Since we're rendering hyperlinks with the helper in Text+HTML.swift, this seems to cause a `precondition failure: cyclic graph` crash. This check prevents that by making sure the cell is only configured once on dequeue.
needs to be called because we're adding a SwiftUI view to the cell's contentView
… so we know which label can be tappable
we don't need to hide it now that there is a new section below it.
…ickstarter/ios-oss into mbl-983-already-reported-label
Codecov Report
@@ Coverage Diff @@
## main #1857 +/- ##
==========================================
- Coverage 83.98% 83.95% -0.04%
==========================================
Files 1287 1289 +2
Lines 117044 117118 +74
Branches 31129 31158 +29
==========================================
+ Hits 98302 98327 +25
- Misses 17655 17702 +47
- Partials 1087 1089 +2
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Generally looks good, though I have some concerns. See comments. Also, a11y for this cell needs to be updated as well (mark ReportProjectCell
as an a11y element and as a button, at least when it's not flagged) but I can fold that in to the other a11y things if you'd prefer.
import UIKit | ||
|
||
internal final class ReportProjectCell: UITableViewCell, ValueCell { | ||
let isIpad = AppEnvironment.current.device.userInterfaceIdiom == .pad |
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.
This should look at the horizontal size class instead. This is important to make layouts look good on iPad split screen, for example.
struct ReportProjectLabelView: View { | ||
let flagged: Bool | ||
|
||
let isIpad = AppEnvironment.current.device.userInterfaceIdiom == .pad |
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.
Same here; look at the size class instead of the device type.
} else { | ||
HStack { | ||
Text(Strings.Report_this_project_to()) | ||
.font(Font(UIFont.ksr_footnote())) |
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'd suggest using .ksr_body(size: 14)
as the font here instead, to match the project subpage cell.
@@ -16,7 +16,7 @@ internal final class ProjectPamphletContentDataSource: ValueCellDataSource { | |||
|
|||
let values = [ | |||
ProjectPamphletSubpage.comments(project.stats.commentsCount as Int?, .first), | |||
ProjectPamphletSubpage.updates(project.stats.updatesCount as Int?, .last) | |||
ProjectPamphletSubpage.updates(project.stats.updatesCount as Int?, .middle) |
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.
nit: This file is marked as deprecated - I'm not sure we need this change (or the file?)
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.
Ah yea, good catch. was just making sure all uses of these cells had the right positioning. I'll undo this change. Looking forward to removing this file completely.
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 think this cell is still clickable, even when it's already flagged. I'm not 100% sure, since I just overwrote the flagged
bool locally to test this (instead of contacting team A to get myself added to the flagged list). Will you double check and make the cell non-interactive if needed?
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.
double checked and the "already reported" label is never tappable. Only when flagged is false do we show the tappable "report this project" label
bc2ac13
to
e7576a2
Compare
e7576a2
to
b38f921
Compare
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.
Size class stuff looks good - thanks for updating! Accessibility still isn't quite there, but I'll leave it up to you if you want to fix that now or not.
let accessibilityTraits = projectFlagged | ||
? UIAccessibilityTraits.staticText | ||
: UIAccessibilityTraits.button | ||
|
||
_ = self | ||
|> baseTableViewCellStyle() | ||
|> ReportProjectCell.lens.accessibilityTraits .~ accessibilityTraits |
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 adding this! Unfortunately, I don't think it does anything currently; we'd need to also set isAccessibilityElement = true
and the accessibility label. I don't see an easy way to set the label based on the swiftui child view. So I'd suggest either leaving this alone for now or adding it just for the button state. Ex:
if projectFlagged {
self.accessibilityTraits = super.accessibilityTraits
self.isAccessibilityElement = false
} else {
self.accessibilityTraits.insert(.button)
self.isAccessibilityElement = true
self.accessibilityLabel = Strings.Report_this_project_to()
}
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.
oh, good callout! I'll look into this in the next PR
📲 What
Replace the "Report this project to Kickstarter" cell with a SwiftUI view that dynamically shows either the original label or a label informing the user that they've already reported the project.
🤔 Why
If a user has already reported a project there's no need to do it again.
🛠 How
Instead of adding a label to the
ProjectPamphletSubpage
, we can build the new labels with SwiftUI and add it as its own section to the project page's table view datasource. Giving us more control and more SwiftUI.Uses the new
flagging
property on Projects to conditionally render the labels.The new "Already reported" label should not be tappable and include 2 hyperlinks.
Followed Android's implementation as a design reference.
👀 See
✅ Acceptance criteria
You'll need to ask someone on web team A to add your staging account email to the project "Undercover:: The cover art of underglow comics - 200+ pages". This is a test project that has been marked as flagged, but you'll still need it associated with your account so that you see the "Already reported" label.
Also, I didn't choose which project to test with so sorry if that projects cover art is a bit saucy.
Otherwise, any other project will show the "Report this project" label as normal