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

New Tab Page post-review fixes #3221

Merged
merged 44 commits into from
Sep 16, 2024
Merged

New Tab Page post-review fixes #3221

merged 44 commits into from
Sep 16, 2024

Conversation

dus7
Copy link
Contributor

@dus7 dus7 commented Aug 9, 2024

Task/Issue URL: https://app.asana.com/0/72649045549333/1208023858138598/f
Tech Design URL:
CC:

Description:

This PR contains fixes for issues and glitches found for New Tab Page Improvements.

Steps to test this PR:
No specific steps but rather a set of things that were fixed/modified. Overall feature smoke check is required.

  1. Missing translations for options in NTP settings.
  2. Favorites blinking when NTP becomes visible.
  3. Display issues on reordering shortcuts in settings.
  4. Animations glitches for expand button and favorites/shortcuts collections.
  5. Customize button placement based on the contents.
  6. NTP content margins and layout.
  7. Spacings and list separators padding in NTP Settings.
  8. Add haptics on items reordering.
  9. Dark mode color for section settings icon.
  10. Color adjustments for shortcuts in edit mode.
  11. NTP empty state layout.
  12. Grid layout based on orientation.
  13. Grid padding / item spacing.
  14. A few code-related improvements.

For a full list and details see completed subtasks of https://app.asana.com/0/72649045549333/1207539163549343/f.

Definition of Done (Internal Only):

Copy Testing:

  • Use of correct apostrophes in new copy, ie rather than '

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

Copy link

github-actions bot commented Aug 9, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.
Messages
📖

You seem to be updating localized strings. Make sure that you request translations and include translated strings before you ship your change. See Localization Guidelines for more information.

Generated by 🚫 dangerJS against 6160454

@dus7 dus7 marked this pull request as ready for review September 12, 2024 09:28
@dus7 dus7 requested a review from brindy September 12, 2024 09:28
@@ -24,15 +24,14 @@ import Common

struct FaviconsHelper {

private static let tld: TLD = AppDependencyProvider.shared.storageCache.tld
private static let tld: TLD = AppDependencyProvider.shared.storageCache.tld

static func loadFaviconSync(forDomain domain: String?,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function says sync but there was no way to use the function without completion. I modified this one and added a version with completion for backwards compatibility.

isLocalFlagEnabled && isFeatureFlagEnabled
let isLocalFlagInEffect = isLocalFlagEnabled && internalUserDecider.isInternalUser

return isLocalFlagInEffect || isFeatureFlagEnabled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remote flag is now .remoteDevelopment, so either local flag needs to be set or enabled in privacy config, both only for internal users.

@brindy brindy self-assigned this Sep 13, 2024
Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

LGTM! Smoke tested on phone and iPad simulators.

@brindy brindy removed their assignment Sep 15, 2024
@dus7 dus7 merged commit 9ed705c into main Sep 16, 2024
26 checks passed
@dus7 dus7 deleted the mariusz/ntp-ship-review branch September 16, 2024 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants