[MBL-902] [MBL-934] [MBL-928] Use of AI Screen, Analytics, Question Headers, Translations #1843

msadoon · 2023-08-10T03:07:20Z

📲 What

Biggest piece of the puzzle is to attach the data to the UI. These were the designs followed:

🤔 Why

We need to display the UI on this tab.

🛠 How

Added the header title. Needs translation.
Repurposed a few cells from environmental commitments for generated and other AI fields. ProjectEnvironmentalCommitmentCell --> ProjectTabCategoryDescriptionCell
ProjectEnvironmentalCommitmentDisclaimerCell --> ProjectTabDisclaimerCell
Added ProjectTabCheckmarkListCell for the funding fields.
Updated ProjectPageViewControllerDataSource to have the new sections. This is the main piece of this PR. It reloads the table for each tab change. Probably the most important thing to understand for the how the project page works.
Everything kind of follows from the above.

👀 See

Before 🐛

Simulator.Screen.Recording.-.iPhone.8.-.2023-08-08.at.20.00.38.mp4

After �

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-08-22.at.13.21.34.mp4

Simulator.Screen.Recording.-.iPhone.14.Pro.Max.-.2023-08-22.at.13.22.41.mp4

Note: You'll notice the check mark is slightly above the text and it looks a little funny. I spent hours trying to pad the UIImageView and Image top. If either of you have any suggestions please try it out and let me know.

*Note: This ticket blew up a bit due to time constraints. All things listed in TODO now encapsulate the completed feature tab.

✅ Acceptance criteria

No regressions to other tabs, new tab has all sections in design above. Sample project is called Paragon Park: A Documentary in staging.

⏰ TODO

Just like you found, link for the bottom disclaimer needs updating - https://help.kickstarter.com/hc/en-us/articles/16848396410267 is the hard link.
Strengthen logic for showing the tab ProjectNavigationSelectorViewModel line 68. “if the AI data is present, and if there is no AI data present on a project, do not display the tab” that is exactly how the current implementation works, with the addition that, if the feature flag is not enabled, the tab will never be visible. Right now I just check the data is there, but there's a boolean check that should override this, it's one of the returned properties.
Translations done by @ifosli
Analytics - "use_of_ai" for Page Viewed segment event.
Add the question headers as per this ticket.
Write tests. Look for FIXME:// MBL-902 for pointers but there's probably more. General strategy is to look at how the each file was tested previously and extend the tests for this tab.

…arter/ios-oss into mbl-900/new-tab-use-of-ai

…ue in the check mark I will note.

nativeksr · 2023-08-15T15:46:22Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

Kickstarter-iOS/Features/ProjectPage/Views/Cells/ProjectTabCheckmarkListCell.swift

Kickstarter-iOS/Features/ProjectPage/Views/Cells/ProjectTabDisclaimerCell.swift

Library/HelpType.swift

…ence of data.

…rter/ios-oss into mbl-902/use-of-ai-screen

…ely represented here.

…rter/ios-oss into mbl-902/use-of-ai-screen

ifosli

Overall, I think this is almost done! We're just missing the green bars (which should be on the leading edge of the answers only). I like that you split the title into its own cell, but now both that title cell and the other AI page cells can be simplified. Some of this can probably be polish follow-up work if needed, but please take a look at my comments, especially the feature flag check that I think has been removed.

ifosli · 2023-08-22T19:33:09Z

...Tests/testLoggedOut_NonBacker_LiveProjectSwitchedToUseOfAITab_Success.lang_de_device_pad.png

I think we're missing a separator between the generation cell and the other cell - it looks weird to have a separator between the first two cells but not the second two. Or, alternatively, we can get rid of the separator after the first cell, and just keep the footer ones.

The separator logic is a little tricky to hammer out right now.

Basically every cell has a separator, but each cell can be re-used. So example the ProjectQuestionAndAnswerCell is used twice with the last cell having the separator. I could technically put in some logic to update the UI for the last cell and ignore it for the others, but given the time constraints on this, can I suggest it be an improvement along with the green bar?

I'm okay with fixing this later. I'd rather have missing separators than have a separator between the questions in the meanwhile, especially since that separator doesn't respect the leading margin.

ifosli · 2023-08-22T19:39:50Z

...ter-iOS/Features/ProjectPage/Datasource/Datasource/ProjectPageViewControllerDataSource.swift

+      if aiDisclosure.funding.fundingForAiAttribution || aiDisclosure.funding
+        .fundingForAiConsent || aiDisclosure.funding.fundingForAiOption {


I think we should be checking that .involvesFunding is true here, in addition to this check.

ifosli · 2023-08-22T19:41:23Z

...ter-iOS/Features/ProjectPage/Datasource/Datasource/ProjectPageViewControllerDataSource.swift

+      self.appendRow(
+        value: Strings.I_plan_to_use_AI_generated_content(),
+        cellClass: ProjectTabTitleCell.self,
+        toSection: Section.aiDisclosureGenerated.rawValue
+      )


I think we should only add this section if .involvesGeneration is true

ifosli · 2023-08-22T19:42:08Z

...ter-iOS/Features/ProjectPage/Datasource/Datasource/ProjectPageViewControllerDataSource.swift

+          )
+      }
+
+      if let otherAIDetailValues = aiDisclosure.otherAiDetails {


And here we should also check that .involvesOther is true

ifosli · 2023-08-22T19:53:16Z

Library/ViewModels/ProjectNavigationSelectorViewModel.swift

@@ -65,8 +65,13 @@ public final class ProjectNavigationSelectorViewModel: ProjectNavigationSelector

      let moreTabs: [NavigationSection] = [.campaign, .faq, .risks]

-      let includeAIDisclosure = extendedProjectProperties
-        .aiDisclosure != nil && featureUseOfAIProjectTabEnabled()


Is this not going to roll out behind a feature flag after all? I'm concerned.

Yes! completely missed this, nice catch! Really important actually.

ifosli · 2023-08-22T19:55:54Z

Kickstarter-iOS/Features/ProjectPage/Views/Cells/ProjectTabTitleCell.swift

I'm not opposed to this title being its own cell, but I think we should use it for all the titles instead of letting the checkmark and category description cells have their own copies.

I'm also confused by why it still has a stack view when the stack view only contains one element.

yea, I used a stackview because it was simpler to re-use the code from ProjectFAQCell alongside styling that currently existed.

We can rework all title cells to be there own cell, sure - but it's a lot of overhead on this time-constrainted PR. We want to kind of limit the scope of changes to this tab - not all possible cells in every tab.

While the difference between title as its own cell or incorporated into other cells isn't noticeable visually, it's still different. In voiceover, for example, it's very obvious and pretty weird actually. I really think we should be consistent across both the ai tab and the environmental tab. I agree that it's a bigger change than what's currently in this PR (hence why I didn't think I could land the questions/answers in this 5.10). Doing it the way you're currently doing it (keeping the stack view and having the title be its own cell in only one place) is quick and simple, but it's adding tech debt. I don't think we want these changes to live like this in the long run, but I'm okay with doing it this way in this pr (though ideally with added FIXME tags) as long as we're planning to clean it up asap. I don't think our app is in a state where we want to create more tech debt.

I hear you on the tech debt. Honestly though the way we have UIKit used in the app (alongside Prelude) is all tech debt. We should be using SwiftUI. To remove all the tech debt in this title cell and the others, we should be building those views in SwiftUI.

ifosli · 2023-08-22T20:00:35Z

Kickstarter-iOS/Features/ProjectPage/Views/Cells/ProjectTabQuestionAnswerCell.swift

The question is indented weirdly and the answer is missing the green bar. I think if we need to do the green bars as a fast follow for the next release, we probably can, but can you fix the indentation?

Sure, I'll remove the indent - I thought it might be good as an indicator for different questions from title. If we're doing a green bar though I'll remove the indent.

The green bar is only along the answer, not the questions. I think the title and the question are primarily differentiated by their different fonts. I don't remember exactly what they looked like on android, though.

codecov · 2023-08-22T20:16:49Z

Codecov Report

Merging #1843 (7973635) into main (9a91c6b) will increase coverage by 0.07%.
The diff coverage is 94.79%.

@@            Coverage Diff             @@
##             main    #1843      +/-   ##
==========================================
+ Coverage   84.54%   84.61%   +0.07%     
==========================================
  Files        1272     1279       +7     
  Lines      115378   116126     +748     
  Branches    30710    30921     +211     
==========================================
+ Hits        97542    98260     +718     
- Misses      16767    16784      +17     
- Partials     1069     1082      +13

Files Changed	Coverage Δ
KsApi/models/ExtendedProjectProperties.swift	`100.00% <ø> (ø)`
...iewModels/ProjectNavigationSelectorViewModel.swift	`92.64% <71.42%> (-1.20%)`	⬇️
...s/ProjectTabCategoryDescriptionCellViewModel.swift	`75.00% <75.00%> (ø)`
...ectPage/Views/Cells/ProjectTabDisclaimerCell.swift	`84.74% <79.06%> (ø)`
...els/graphql/adapters/Project+ProjectFragment.swift	`89.78% <82.22%> (-1.89%)`	⬇️
...ectPage/Controller/ProjectPageViewController.swift	`50.47% <85.71%> (+0.23%)`	⬆️
...dels/ProjectNavigationSelectorViewModelTests.swift	`90.67% <88.23%> (-0.44%)`	⬇️
...tasource/ProjectPageViewControllerDataSource.swift	`86.40% <88.88%> (+1.30%)`	⬆️
.../ProjectPage/Views/Cells/ProjectTabTitleCell.swift	`96.22% <96.22%> (ø)`
...age/Views/Cells/ProjectTabQuestionAnswerCell.swift	`97.05% <97.05%> (ø)`
... and 20 more

... and 1 file with indirect coverage changes

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

…rter/ios-oss into mbl-902/use-of-ai-screen

ifosli

Looks good! I think all remaining work can happen in the next cut now :)

msadoon added 9 commits August 2, 2023 15:55

Updated model with ai disclosure fields

bfde6bc

added use of ai tab

bb2c048

Merge branch 'main' into mbl-900/new-tab-use-of-ai

32dd553

Merge branch 'main' into mbl-900/new-tab-use-of-ai

b879f74

fixed a test for use of ai tab title

f913b88

Merge branch 'mbl-900/new-tab-use-of-ai' of https://github.com/kickst…

e32af42

…arter/ios-oss into mbl-900/new-tab-use-of-ai

formatting

ad87197

updated all cells for use of ai tab except the funding ones. Up next.

f10b5dd

added final cell and tested - pretty close to design, one padding iss…

aafa8a2

…ue in the check mark I will note.

msadoon added this to the release-5.10.0 milestone Aug 10, 2023

msadoon self-assigned this Aug 10, 2023

msadoon changed the base branch from main to mbl-900/new-tab-use-of-ai August 10, 2023 03:07

msadoon added the WIP label Aug 10, 2023

msadoon changed the base branch from mbl-900/new-tab-use-of-ai to main August 10, 2023 03:30

msadoon changed the title ~~[MBL 902] Use of AI Screen~~ [MBL 902] Use of AI Screen [wait until MBL 900 merged] Aug 10, 2023

msadoon changed the title ~~[MBL 902] Use of AI Screen [wait until MBL 900 merged]~~ [MBL 902] Use of AI Screen Aug 15, 2023

msadoon assigned ifosli Aug 15, 2023

Merge branch 'main' into mbl-902/use-of-ai-screen

7bb8164

ifosli added 2 commits August 16, 2023 10:27

Stop using stack view in order to align checkmark and label

590d75f

Update footer to link to AI policy

0d66016

msadoon commented Aug 17, 2023

View reviewed changes

Kickstarter-iOS/Features/ProjectPage/Views/Cells/ProjectTabCheckmarkListCell.swift Show resolved Hide resolved

msadoon commented Aug 17, 2023

View reviewed changes

Kickstarter-iOS/Features/ProjectPage/Views/Cells/ProjectTabDisclaimerCell.swift Outdated Show resolved Hide resolved

msadoon commented Aug 17, 2023

View reviewed changes

Library/HelpType.swift Show resolved Hide resolved

msadoon commented Aug 17, 2023

View reviewed changes

Library/HelpType.swift Show resolved Hide resolved

ifosli and others added 5 commits August 17, 2023 13:08

Update link to have plain text default

fecc868

Merge branch 'main' into mbl-902/use-of-ai-screen

06b15b2

Use translatable strings

de7f8b2

added analytics, checking if involves ai is true along with the exist…

b84815b

…ence of data.

Merge branch 'mbl-902/use-of-ai-screen' of https://github.com/kicksta…

90764de

…rter/ios-oss into mbl-902/use-of-ai-screen

msadoon changed the title ~~[MBL 902] Use of AI Screen~~ [MBL-902] [MBL-934] [MBL-928] Use of AI Screen, Analytics, Question Headers, Translations Aug 21, 2023

ifosli and others added 6 commits August 22, 2023 09:43

Add a11y label so voiceover reads tab correctly

15e8a20

Added titles to questions and answers, all sections should be accurat…

84b77b1

…ely represented here.

Merge branch 'mbl-902/use-of-ai-screen' of https://github.com/kicksta…

55736ad

…rter/ios-oss into mbl-902/use-of-ai-screen

correct to title headers and order

c433f53

tested kickstarter-ios and library

2b17624

ksapi changes tested

2a0559f

msadoon requested review from scottkicks and ifosli August 22, 2023 19:21

msadoon added needs review and removed WIP labels Aug 22, 2023

Merge branch 'main' into mbl-902/use-of-ai-screen

f657b3c

msadoon marked this pull request as ready for review August 22, 2023 19:22

ifosli reviewed Aug 22, 2023

View reviewed changes

msadoon added 2 commits August 22, 2023 17:45

pr comments

923813b

Merge branch 'mbl-902/use-of-ai-screen' of https://github.com/kicksta…

9cfd437

…rter/ios-oss into mbl-902/use-of-ai-screen

msadoon requested a review from ifosli August 22, 2023 21:48

removing cell separator and added a FIXME for title cell

7973635

ifosli approved these changes Aug 22, 2023

View reviewed changes

msadoon merged commit 3d464ee into main Aug 22, 2023
7 checks passed

msadoon deleted the mbl-902/use-of-ai-screen branch August 22, 2023 23:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MBL-902] [MBL-934] [MBL-928] Use of AI Screen, Analytics, Question Headers, Translations #1843

[MBL-902] [MBL-934] [MBL-928] Use of AI Screen, Analytics, Question Headers, Translations #1843

msadoon commented Aug 10, 2023 •

edited

nativeksr commented Aug 15, 2023 •

edited

ifosli left a comment

ifosli Aug 22, 2023

msadoon Aug 22, 2023 •

edited

ifosli Aug 22, 2023

ifosli Aug 22, 2023

ifosli Aug 22, 2023

ifosli Aug 22, 2023

ifosli Aug 22, 2023

msadoon Aug 22, 2023

ifosli Aug 22, 2023

msadoon Aug 22, 2023

ifosli Aug 22, 2023

msadoon Aug 22, 2023

ifosli Aug 22, 2023

msadoon Aug 22, 2023 •

edited

ifosli Aug 22, 2023

codecov bot commented Aug 22, 2023 •

edited

ifosli left a comment

		if aiDisclosure.funding.fundingForAiAttribution \|\| aiDisclosure.funding
		.fundingForAiConsent \|\| aiDisclosure.funding.fundingForAiOption {

[MBL-902] [MBL-934] [MBL-928] Use of AI Screen, Analytics, Question Headers, Translations #1843

[MBL-902] [MBL-934] [MBL-928] Use of AI Screen, Analytics, Question Headers, Translations #1843

Conversation

msadoon commented Aug 10, 2023 • edited

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

⏰ TODO

nativeksr commented Aug 15, 2023 • edited

ifosli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msadoon Aug 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msadoon Aug 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2023 • edited

Codecov Report

ifosli left a comment

Choose a reason for hiding this comment

msadoon commented Aug 10, 2023 •

edited

nativeksr commented Aug 15, 2023 •

edited

msadoon Aug 22, 2023 •

edited

msadoon Aug 22, 2023 •

edited

codecov bot commented Aug 22, 2023 •

edited