Skip to content
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.

Sign up for GitHub

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

Jump to bottom

(Android) Redesign private tab UI #25850

Merged
merged 31 commits into from
Oct 15, 2024
Merged

(Android) Redesign private tab UI #25850

merged 31 commits into from
Oct 15, 2024

Conversation

simoarpe
Copy link
Collaborator

@simoarpe simoarpe commented Oct 7, 2024 •
edited
Loading

Resolves brave/brave-browser#37596

Redesign private tab UI following Figma design.

Private tab redesign includes three new sections "Leave no trace", "Browsing visibility" and "Brave VPN".
Brave VPN section will show a call to action text view that will redirect to the subscription page. If the user is already a paying customer, or the VPN is not supported the CTA view will be hidden.

The design is responsive and adjust to large device screens and tablets, following the logic provided by Figma design.
Under normal conditions the entire layout is centered in the visible part of the page.

Smartphone

Layout Bounds OFF Layout Bounds ON
Screenshot 2024-10-15 at 4 18 21 PM Screenshot 2024-10-15 at 4 57 14 PM

Tablet

Layout Bounds OFF Layout Bounds ON
Screenshot 2024-10-15 at 5 02 30 PM Screenshot 2024-10-15 at 5 02 39 PM

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Tap on the overflow menu icon
  • Tap on New private tab
  • Observe the private UI loads the new layout containing three blocks
  • Observe the new layout is loaded centered in the visible part of the page
  • Tap on "Try free for 7 days"
  • Observe the VPN subscription page loads correctly

@simoarpe simoarpe added CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x64 Do not run CI builds for Windows x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 labels Oct 7, 2024
@simoarpe simoarpe self-assigned this Oct 7, 2024
@simoarpe simoarpe requested review from deeppandya and a team as code owners October 7, 2024 15:41
mkarolin
mkarolin reviewed Oct 7, 2024
View reviewed changes
browser/ui/android/strings/android_brave_strings.grd Outdated
Browsing visibility
</message>
<message name="IDS_BRAVE_NEW_TAB_PRIVATE_VISIBILITY_DESCRIPTION" desc="Browsing visibility description shown when a user opens a private tab explaining private mode">
Note that -even in private windows- your browsing activity may still be visible to your ISP, network observers, or your employer (if you're using a work machine/network).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we say "device" instead of "machine" since this is only applicable to Android?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I'm changing it!

patches/chrome-android-BUILD.gn.patch Outdated
Comment on lines 49 to 51
+ sources -= [
+ "javatests/src/org/chromium/chrome/browser/ntp/IncognitoDescriptionViewRenderTest.java",
+ ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

We probably want to have a one line here as well where what we remove is defined in sources.gni and here we'd have only sources -= brave_remove_.... Not sure we have a naming convention for it. cc: @bridiver

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed into a single line.

@simoarpe simoarpe requested a review from mkarolin October 9, 2024 13:22
SergeyZhukovsky
SergeyZhukovsky approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

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

++

@simoarpe simoarpe force-pushed the simone/private-tab-redesign branch 2 times, most recently from f937798 to 02b56f2 Compare October 10, 2024 10:07
samartnik
samartnik approved these changes Oct 10, 2024
View reviewed changes
Copy link
Contributor

@samartnik samartnik left a comment

Choose a reason for hiding this comment

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

lgtm

mkarolin
mkarolin approved these changes Oct 10, 2024
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

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

++

deeppandya
deeppandya reviewed Oct 11, 2024
View reviewed changes
Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

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

Looks good overall. we need to update the style based on figma for new UI. just checked with agustin and it seems it was missed initially.

@simoarpe
Copy link
Collaborator Author

@deeppandya is there anything I have to update in this PR? (Other than the conflicts, of course)

@deeppandya
Copy link
Contributor

@deeppandya is there anything I have to update in this PR? (Other than the conflicts, of course)

Style details were missing in dogma. So instead of using text size, etc we should use style from figma to make things uniform.

@simoarpe
Copy link
Collaborator Author

@deeppandya got it.
I'm not sure we have the right styles for those. All the text views in that layout need to be unaffected by theme changes as they are always in that specific color independently by the day/night mode used.

@simoarpe
Copy link
Collaborator Author

@deeppandya changes addressed in 94a6506 (#25850)

deeppandya
deeppandya approved these changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

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

LGTM

@simoarpe simoarpe merged commit 7a362dd into master Oct 15, 2024
17 checks passed
@simoarpe simoarpe deleted the simone/private-tab-redesign branch October 15, 2024 17:18
@github-actions github-actions bot added this to the 1.73.x - Nightly milestone Oct 15, 2024
@brave-builds
Copy link
Collaborator

Released in v1.73.14

@bsclifton bsclifton modified the milestones: 1.73.x - Nightly, 1.74.x - Nightly Nov 6, 2024
@hffvld hffvld mentioned this pull request Nov 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aguscruiz aguscruiz aguscruiz left review comments

@mkarolin mkarolin mkarolin approved these changes

@samartnik samartnik samartnik approved these changes

@SergeyZhukovsky SergeyZhukovsky SergeyZhukovsky approved these changes

@deeppandya deeppandya deeppandya approved these changes

Assignees

@simoarpe simoarpe

Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-arm64 Do not run CI builds for macOS arm64 CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-windows-x64 Do not run CI builds for Windows x64 puLL-Merge
Projects
None yet
Milestone
1.74.x - Beta
Development

Successfully merging this pull request may close these issues.

Private windows redesign
8 participants
@simoarpe @deeppandya @aguscruiz @brave-builds @SergeyZhukovsky @samartnik @mkarolin @bsclifton