Skip to content
This repository was archived by the owner on May 10, 2024. It is now read-only.

Fix#2980 / Fix#2081: Increase App Virality - Educational in-product Notification (Part 1 - UI) #3139

Closed
wants to merge 13 commits into from
Closed

Fix#2980 / Fix#2081: Increase App Virality - Educational in-product Notification (Part 1 - UI) #3139

wants to merge 13 commits into from

Conversation

soner-yuksel
Copy link
Contributor

@soner-yuksel soner-yuksel commented Dec 11, 2020 •
edited
Loading

This Pull Request is implementing the changes on Brave Shields to promote sharing (Increase App Virality) and new Brave Shields Ongoing Education Screen Types.

The implementation currently includes UI and action points to the respected recipients inside the code base.

Steps to add:

  • Creating the Tracker Tier and show the Share Controller accordingly.
  • Actual Sharing Part (WIP on design side)

Summary of Changes

This pull request fixes #2980
This pull request fixes #2981

Submitter Checklist:

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

Screenshots:

IMG_6194

IMG_6198

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).

@soner-yuksel soner-yuksel added the blocked: needs design Needs design before work can commence label Dec 11, 2020
@soner-yuksel soner-yuksel added this to the 1.23 milestone Dec 11, 2020
@soner-yuksel soner-yuksel self-assigned this Dec 11, 2020
@soner-yuksel soner-yuksel changed the title Feature#2980 - #2081: Increase App Virality - Educational in-product Notification Fix#2980 / Fix#2081: Increase App Virality - Educational in-product Notification Dec 11, 2020
@soner-yuksel soner-yuksel changed the title Fix#2980 / Fix#2081: Increase App Virality - Educational in-product Notification Fix#2980 / Fix#2081: Increase App Virality - Educational in-product Notification (UI) Dec 11, 2020
iccub
iccub suggested changes Dec 11, 2020
View reviewed changes
Copy link
Contributor

@iccub iccub left a comment

Choose a reason for hiding this comment

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

Haven't run the code yet, left initial code review, looks good in general

Client/Extensions/NSAttributedStringExtensions.swift
/// Add Line Spacing to Text
func withLineSpacing(_ spacing: CGFloat) -> NSAttributedString {
let paragraphStyle = NSMutableParagraphStyle()
paragraphStyle.lineSpacing = spacing
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually don't change line spacing even if Figma asks us to do so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this part I think it is needed to make it look good actually but If this is a must, I will revert the line spacing.

@iccub
Copy link
Contributor

iccub commented Dec 11, 2020 •
edited by soner-yuksel
Loading

Ran the app, found 2 things, have checked new shields screen only so far

  1. We have to rethink the UI on small phones, does not look good

1

  1. Minor item: share screen should be scrollable, it can't be scrolled down on small phone in landscape.

3221

@soner-yuksel soner-yuksel changed the title Fix#2980 / Fix#2081: Increase App Virality - Educational in-product Notification (UI) Fix#2980 / Fix#2081: Increase App Virality - Educational in-product Notification (Part 1 - UI) Dec 15, 2020
@soner-yuksel
Copy link
Contributor Author

soner-yuksel commented Dec 15, 2020 •
edited
Loading

Ran the app, found 2 things, have checked new shields screen only so far

  1. We have to rethink the UI on small phones, does not look good

This design is not appropriate for small phones like iPhone SE 1st Gen, I will discuss this with design

  1. Minor item: share screen should be scrollable, it can't be scrolled down on small phone in landscape.

I will make that part scrollable for landscape

@soner-yuksel soner-yuksel marked this pull request as ready for review December 15, 2020 18:19
@soner-yuksel soner-yuksel mentioned this pull request Dec 15, 2020
Merged
7 tasks
Brandon-T
Brandon-T suggested changes Dec 16, 2020
View reviewed changes
@soner-yuksel
Copy link
Contributor Author

Closing this PR to create a new one in order to separate Issues #2980 #2981 cause implementation of share functionality is blocked by design

@soner-yuksel soner-yuksel deleted the increase-app-virality branch April 6, 2021 14:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@iccub iccub iccub requested changes

@kylehickinson kylehickinson Awaiting requested review from kylehickinson

@Brandon-T Brandon-T Awaiting requested review from Brandon-T

Assignees

@soner-yuksel soner-yuksel

Labels
blocked: needs design Needs design before work can commence
Projects
None yet
Milestone
1.23
Development

Successfully merging this pull request may close these issues.

Increase App Virality Educational in-product notification
3 participants
@soner-yuksel @iccub @Brandon-T