Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[MBL-970] Report Project Info View #1852
Changes from 15 commits
d3b513e
c46c8d9
a23f0ed
42c426b
36f854d
c2b7914
11e5563
dcfc84e
0d4b397
73cc7a9
63adaba
9596a13
17b3898
be5c703
3a804c6
806c95d
d4a7de4
7daee5a
020ee58
256e33a
07f8d0e
0548b41
bcdf584
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 might be tied to the weird navigation bar behaviour in the review overview video.
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.
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 thex
, 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.
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 guess we'll be releasing this with the iOS 17 upgrade?
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.
No this is blocking our next release and needs to go out in 5.11.0.
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.
Designs show a disclosure indicator to indicate tappable cell. I don't see one here beside "Report this project to Kickstarter"
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'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.
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.
No internationalized text for this from when Android did it?
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.
We don't have an internationalized string for this, unfortunately. This is mainly for the hyperlink. I'm not sure how Android handles hyperlinks, but they wouldn't have an internationalized string. I'll request one from the translations team, but those may not be done by code freeze.
Seems like something that shouldn't hold up this release. What do you think?
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 don't think this should be a separate nav title under the navigation bar. I think the back button and the title should be on the same navigation bar. It should look like "Updates" or "Comments".
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.
Internationlized text for this?
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 is just a placeholder for the new view that will actually be pushed
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.
Would a
.first(where
function be cleaner and more readable than a for loop?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.
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.
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.
Try to use existing methods of placing urls like
HelpType
url(withBaseUrl
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.
Recent example use case was the
ProjectTabDisclaimerCell