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.

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

Android news orientation card #13203

Merged
merged 1 commit into from
Apr 29, 2022
Merged

Conversation

alexsafe
Copy link
Contributor

@alexsafe alexsafe commented Apr 28, 2022

Resolves brave/brave-browser#22569
Resolves brave/brave-browser#22444

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • 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, npm run lint, 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:

@alexsafe alexsafe added CI/skip-linux CI/skip-macos-x64 Do not run CI builds for macOS x64 CI/skip-ios Do not run CI builds for iOS labels Apr 28, 2022
@alexsafe alexsafe added this to the 1.40.x - Nightly milestone Apr 28, 2022
@alexsafe alexsafe self-assigned this Apr 28, 2022
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment

Choose a reason for hiding this comment

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

++

@@ -732,6 +732,10 @@ private void refreshFeed() {

private void keepPosition(int prevScrollPosition, int prevRecyclerViewPosition,
int prevRecyclerViewItemPosition) {
if ((!mIsNewsOn && mIsShowNewsOn) || (mIsNewsOn && mIsShowOptin)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not subject or request to change, but the fact which may be useful to understand the intention:

(!mIsNewsOn && mIsShowNewsOn) || (mIsNewsOn && !mIsShowNewsOn)
is equal algebraically to just
(mIsNewsOn != mIsShowNewsOn)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AlexeyBarabash thanks! that's a good point. updating

also fixes news feed wrongly appearing on orientation change after disabling
@alexsafe alexsafe force-pushed the android-news-orientation-card branch from c19c69a to 3548837 Compare April 29, 2022 14:54
@alexsafe alexsafe merged commit 7a7eaba into master Apr 29, 2022
@alexsafe alexsafe deleted the android-news-orientation-card branch April 29, 2022 15:36
brave-builds pushed a commit that referenced this pull request May 4, 2022
@kjozwiak
Copy link
Member

kjozwiak commented May 11, 2022

Verification PASSED on Samsung S10+ running Android 12 using the following build(s):

Brave | 1.40.44 Chromium: 101.0.4951.54 (Official Build) canary (32-bit)
--- | ---
Revision | 67da1aeb32cedd27634ca6634fb79cbd85d3f0ab-refs/branch-heads/4951@{#1126}
OS | Android 12; Build/SP1A.210812.016

Test Case #1 - Running through various opt-in scenarios

New Installs/Fresh Profile

  • ensured that the Brave News opt-in card is somewhat visible under NTP
  • ensured that scrolling down displays the entire Brave News opt-in card
  • ensured that clicking on x dismisses the card and doesn't re-appear when opening NTP tabs
  • ensured that the Brave News opt-in card didn't re-appear when dismissed and the browser was restarted
  • ensured that users can enable Brave News via the Hamburger Menu -> Brave News if the card was already dismissed
  • ensured that users can enable Brave News via Settings -> Brave News if the card was already dismissed
  • ensured that disabling/enabling Brave News doesn't display the opt-in card via NTP as the user already opted-in
  • ensured that tapping on Learn more about your data opens up https://brave.com/privacy/browser
  • ensured that tapping on Show Brave News via the opt-in card enables/displays the Brave News feed
Example Example Example
Screenshot_20220510-210720_Brave - Nightly Screenshot_20220510-210727_Brave - Nightly Screenshot_20220510-210903_Brave - Nightly

Upgrading (Brave News hasn't been enabled)

  • downloaded/installed 1.39.67 Chromium: 101.0.4951.34 and ensured that the opt-in card isn't being displayed under NTP
  • upgraded to 1.40.44 Chromium: 101.0.4951.54 and ensured that the opt-in card is being displayed without any issues
  • ensured that once upgraded, the user can opt-in into Brave News by tapping on Show Brave News
  • ensured that once upgraded, users can dismiss the opt-in card via NTP and later enable Brave News via Settings

Upgrading (Brave News enabled but then disabled by user)

  • downloaded/installed 1.39.67 Chromium: 101.0.4951.34
  • enabled Brave News via Settings and then disabled the the feature via Settings (ensured feed isn't appearing under NTP)
  • upgraded to 1.40.44 Chromium: 101.0.4951.54
  • ensured that the opt-in card wasn't being displayed as the user already enabled Brave News but decided to disable
  • ensure that you can re-enable Brave News via Settings without any issues after upgrading

Upgrading (Brave News enabled & visible)

  • downloaded/installed 1.39.67 Chromium: 101.0.4951.34 and enabled Brave News via Settings
  • upgraded to 1.40.12 Chromium: 101.0.4951.41
  • ensured that Brave News is still enabled/appearing under NTP

Created/Ran into the following issues while going through the above cases:

Test Case #2 - brave/brave-browser#22444

Went through the STR/Cases outlined via brave/brave-browser#22444 (comment) and ensured that the original issue wasn't occurring.

  • ensured that the news feed isn't being displayed within the same tab once disabled and device orientation is changed
  • ensured that the news feed isn't being displayed in new NTP when changing device orientation once disabled
  • ensured that restarting the browser and changing the orientation on the NTP doesn't display news after it's been disabled
  • ensured that switching between portrait & landscape several times under NTP doesn't cause issues when news has never been enabled (basically the user dismissing the onboarding card)
  • ensured that switching between portrait & landscape several times under NTP doesn't cause issues when news was enabled and then disabled

Quick example of the issue not occurring anymore:

Screen_Recording_20220510-202216_Brave.-.Nightly.mp4

Test Case #3 - brave/brave-browser#22569

Went through the STR/Cases outlined via brave/brave-browser#22569 (comment) and ensured that the original issue wasn't occurring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI/skip-ios Do not run CI builds for iOS CI/skip-macos-x64 Do not run CI builds for macOS x64
Projects
None yet
3 participants