[WIP] Fix #2981: Increase App Virality #3161

soner-yuksel · 2020-12-23T18:46:33Z

This Pull Request is implementing the changes on Brave Shields to promote sharing (Increase App Virality)

The implementation currently adding all App Virality features like creating the tracker tier and changes to educational onboarding and changes to the Brave shield screen

Steps to add:
Actual Sharing Part (WIP on design side)

Summary of Changes

This pull request fixes #2981

Submitter Checklist:

Unit Tests are updated to cover new or changed functionality
User-facing strings use NSLocalizableString()

Test Plan:

Press Brave Shields on Top Bar
Checkout the changes on blocked row
Press the share button and checkout the share type screen with row types

Screenshots:

Reviewer Checklist:

Issues include necessary QA labels:
- QA/(Yes|No)
- release-notes/(include|exclude)
- bug / enhancement
Necessary security reviews have taken place.
Adequate unit test coverage exists to prevent regressions.
Adequate test plan exists for QA to validate (if applicable).
Issue is assigned to a milestone (should happen at merge time).

iccub

Code looks super clean, left few questions and code style feedback, will run the code and leave more feedback later,

this PR would be good to be reviewed by @kylehickinson as well as he's worked on the revamped shields panel

BraveShared/BraveStrings.swift

BraveShared/Extensions/UIStackViewExtensions.swift

Client/Frontend/Browser/BrowserViewController/BrowserViewController+ProductNotification.swift

iccub · 2021-01-04T16:04:26Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+ProductNotification.swift

+            }
+        }
+
+        var nextTier: BenchmarkTrackerCountTier? {


this could get a unit test, to check next value and last value(all tiers passed)

Tests are added 1e20931

Client/Frontend/Shields/ShareShieldsController.swift

Client/Frontend/Shields/ShieldsViewController.swift

Client/Frontend/Shields/SimpleShieldsView.swift

iccub · 2021-01-04T16:56:26Z

I did not notice that this pull request is based on another opened PR, few of my comments might be not applicable then

kylehickinson

Really great UI code :) A couple of nits here and there. One thing you may need to look out for, in addition to review comments, is iOS 12 theming (make sure it works correctly) and accessibility/VoiceOver support.

BraveShared/BraveStrings.swift

BraveShared/Extensions/UIStackViewExtensions.swift

Client/Extensions/UIViewExtensions.swift

kylehickinson · 2021-01-04T16:41:35Z

Client/Frontend/Browser/BrowserViewController/BrowserViewController+ProductNotification.swift

+        benchmarkNotificationPresented = false
+
+        dismiss(animated: true) {
+            // TODO: Share with Email


Are we waiting on product/design decision on sharing implementations? Perhaps may want to switch to WIP/Draft PR

Yes this is waiting for product/design to finish final sharing part.
I added information in the description about it and marked it as Blocked

Client/Frontend/Shields/ShareShieldsController.swift

Client/Frontend/Shields/SimpleShieldsView.swift

kylehickinson · 2021-01-11T19:06:30Z

Client/Frontend/Shields/ShareShieldsController.swift

+
+    override func accessibilityElementCount() -> Int {
+        return 1
+    }
+
+    override func accessibilityElement(at index: Int) -> Any? {
+        switch index {
+        case 0:
+            return titleLabel
+        default:
+            return nil
+        }
+    }


How come this is needed? I've never needed this personally to setup a count + element

The view has a container view and inside there are some elements. So what I wanted to achieve here is use title label text as accessibilityElement regardless of what has been added inside and will be added later.

…om Title of Description

soner-yuksel · 2021-03-02T19:59:14Z

Closing this PR in order to Open a new PR with UI/feature spec changes

soner-yuksel added the blocked: needs info Needs further information before work can commence label Dec 23, 2020

soner-yuksel added this to the 1.23 milestone Dec 23, 2020

soner-yuksel requested review from kylehickinson, Brandon-T and iccub December 23, 2020 18:46

soner-yuksel self-assigned this Dec 23, 2020

soner-yuksel linked an issue Dec 23, 2020 that may be closed by this pull request

Increase App Virality #2981

Closed

soner-yuksel force-pushed the app-virality-increase branch from b529218 to f8dd684 Compare December 23, 2020 20:14

Brandon-T approved these changes Dec 23, 2020

View reviewed changes

soner-yuksel force-pushed the app-virality-increase branch from f8dd684 to 8576926 Compare December 28, 2020 20:13

iccub suggested changes Jan 4, 2021

View reviewed changes

kylehickinson suggested changes Jan 4, 2021

View reviewed changes

soner-yuksel force-pushed the educational-product-notification branch from 01f4b5f to ff510f9 Compare January 6, 2021 18:05

soner-yuksel force-pushed the app-virality-increase branch 2 times, most recently from 75d6ddf to 8661b2d Compare January 8, 2021 19:56

kylehickinson reviewed Jan 11, 2021

View reviewed changes

kylehickinson changed the title ~~Fix#2081: Increase App Virality~~ Fix #2981: Increase App Virality Jan 11, 2021

soner-yuksel force-pushed the educational-product-notification branch from caa6200 to 5b915db Compare January 11, 2021 19:15

soner-yuksel force-pushed the app-virality-increase branch from de862fe to f21b9d2 Compare January 11, 2021 19:16

kylehickinson removed this from the 1.23 milestone Jan 11, 2021

soner-yuksel force-pushed the app-virality-increase branch from f21b9d2 to 4f900c4 Compare January 11, 2021 20:30

soner-yuksel force-pushed the educational-product-notification branch from 0a570de to 93f7322 Compare January 12, 2021 22:15

soner-yuksel force-pushed the app-virality-increase branch from 4f900c4 to ef576b0 Compare January 12, 2021 22:33

Base automatically changed from educational-product-notification to development January 14, 2021 18:53

soner-yuksel added 5 commits January 14, 2021 16:05

Add Content Holder to BlockCountView and Remove Information Button fr…

c4e107f

…om Title of Description

Adding information button to shield up description

db4173f

Adding the share button into the stack

f2abcfa

Information and share sections are added

802eaaf

Share Types and Screen implementation

10604b8

soner-yuksel added 21 commits January 14, 2021 17:56

View Types are added for Share

02fa8ec

Button Type Share is combined

c9690be

Pull Request Comments are addressed

0f9c0f7

Changing Add Block notification description using 20+

1f0a91b

Feedback and changes related with notifications

34f547d

Pull Request Comments are addressed

54a0a3f

Changing # of ad/blockers warnin

4d397cd

Comments Design Review are addresse

5df369c

Share Strings are added

71a60b1

Stack View add background functionality is added

5e9d62e

Adding different functionality of subviews in UIView

4fe772a

Adding ShareShieldController and beanchmark onboarding interface

e5e783a

Adding info and share changes on brave shields

c63be5a

Updating icons images for information and type share

f974c5b

The Share Icon is fixed

73d01d4

Pull Request Comments are addressed

91d1f69

Enum is creatd with suffix tier

b5a7456

All tiers shown is removed and compilation error is fixed

4aac5c4

Adding Tests to Next Tier Block Status Benchmark

25198f0

Accessibility Elements and iOS 12 theming is added

9ab7700

Adding benchmark type into delay

c7f1b23

soner-yuksel force-pushed the app-virality-increase branch from f2ddf51 to c7f1b23 Compare January 14, 2021 23:10

soner-yuksel added 2 commits January 14, 2021 19:00

Rebase fixes and changes

bc88ae0

Self Review and fixes related with indentation

105cd80

soner-yuksel changed the title ~~Fix #2981: Increase App Virality~~ [WIP} Fix #2981: Increase App Virality Jan 15, 2021

soner-yuksel changed the title ~~[WIP} Fix #2981: Increase App Virality~~ [WIP] Fix #2981: Increase App Virality Jan 15, 2021

soner-yuksel marked this pull request as draft January 15, 2021 00:12

soner-yuksel closed this Mar 2, 2021

soner-yuksel deleted the app-virality-increase branch April 6, 2021 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix #2981: Increase App Virality #3161

[WIP] Fix #2981: Increase App Virality #3161

soner-yuksel commented Dec 23, 2020 •

edited by kylehickinson

Loading

iccub left a comment

iccub Jan 4, 2021

soner-yuksel Jan 8, 2021 •

edited

Loading

iccub commented Jan 4, 2021

kylehickinson left a comment

kylehickinson Jan 4, 2021

soner-yuksel Jan 8, 2021

kylehickinson Jan 11, 2021

soner-yuksel Jan 11, 2021

soner-yuksel commented Mar 2, 2021

[WIP] Fix #2981: Increase App Virality #3161

[WIP] Fix #2981: Increase App Virality #3161

Conversation

soner-yuksel commented Dec 23, 2020 • edited by kylehickinson Loading

Summary of Changes

Submitter Checklist:

Test Plan:

Screenshots:

Reviewer Checklist:

iccub left a comment

Choose a reason for hiding this comment

iccub Jan 4, 2021

Choose a reason for hiding this comment

soner-yuksel Jan 8, 2021 • edited Loading

Choose a reason for hiding this comment

iccub commented Jan 4, 2021

kylehickinson left a comment

Choose a reason for hiding this comment

kylehickinson Jan 4, 2021

Choose a reason for hiding this comment

soner-yuksel Jan 8, 2021

Choose a reason for hiding this comment

kylehickinson Jan 11, 2021

Choose a reason for hiding this comment

soner-yuksel Jan 11, 2021

Choose a reason for hiding this comment

soner-yuksel commented Mar 2, 2021

soner-yuksel commented Dec 23, 2020 •

edited by kylehickinson

Loading

soner-yuksel Jan 8, 2021 •

edited

Loading