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

[MOB-2564] Fix top sites #682

Merged
merged 2 commits into from
May 21, 2024
Merged

[MOB-2564] Fix top sites #682

merged 2 commits into from
May 21, 2024

Conversation

d4r1091
Copy link
Member

@d4r1091 d4r1091 commented May 16, 2024 •
edited by jira bot

MOB-2564

Context

During testing of the latest upgrade build, there have been some issues verified via Browserstack by the QA team, especially affecting the TopSites.

Approach

  • Validate it being a regression based on the last v122 upgrade
  • Realized the main issue was due to a completely different file being created as EcosiaDefaultSuggestedSite and skipped into the pbxproj resolution
  • This led to utilizing the default Firefox's class
  • Approached differently by commenting explicitly the Firefox's suggested sites and replaced with Ecosia's as part of the same file
  • Improved the BundleImageFetcher.swift+ bundle_sites.json workaround to utilize Ecosia's local favicons
Before After
Simulator Screenshot - iPhone 15 Pro - 2024-05-16 at 14 29 27 Simulator Screenshot - iPhone 15 Pro - 2024-05-16 at 14 24 03

Other

Before merging

Checklist

  • I performed some relevant testing on a real device and/or simulator
  • I wrote Unit Tests that confirm the expected behavior

@d4r1091 d4r1091 requested a review from a team May 16, 2024 12:30
lucaschifino
lucaschifino approved these changes May 21, 2024
View reviewed changes
Copy link

@lucaschifino lucaschifino left a comment

Choose a reason for hiding this comment

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

Nice, like this more evident approach 👍

Added one non-blocking naming comment

BrowserKit/Sources/SiteImageView/ImageProcessing/BundleImageFetcher/BundleImageFetcher.swift Outdated
return "blog.ecosia.finance"
let financialReportsURL = Environment.current.urlProvider.financialReports.absoluteString.replacingOccurrences(of: "https://", with: "")
let privacyURL = Environment.current.urlProvider.privacy.absoluteString.replacingOccurrences(of: "https://", with: "")
let foo = [financialReportsURL : "blog.ecosia.finance",
Copy link

Choose a reason for hiding this comment

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

Did you mean to leave foo here? Would personally prefer something more descriptive like urlMap.

Copy link
Member Author

@d4r1091 d4r1091 May 21, 2024 •
edited

Choose a reason for hiding this comment

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

@lucaschifino definitely slipped! Renaming it at lightning speed!

@d4r1091 d4r1091 merged commit d6499ce into MOB-2012_firefox_120_upgrade May 21, 2024
@d4r1091 d4r1091 deleted the MOB-2564_fix_top_sites branch May 21, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucaschifino lucaschifino lucaschifino approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@d4r1091 @lucaschifino