-
Notifications
You must be signed in to change notification settings - Fork 40
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some questions inline; otherwise looks solid to me
@@ -72,6 +72,7 @@ class Constant { | |||
static let usernamePlaceholder = NSLocalizedString("username_placeholder", value: "(no username)", comment: "Placeholder text when there is no username. String should include appropriate open/close parenthetical or similar symbols to indicate this is a placeholder, not a real username.") | |||
static let searchYourEntries = NSLocalizedString("search.placeholder", value: "Search logins", comment: "Placeholder text for search field") | |||
static let emptyListPlaceholder = NSLocalizedString("list.empty", value: "%@ lets you access passwords you’ve already saved to Firefox. To view your logins here, you’ll need to sign in and sync with Firefox.", comment: "Label shown when there are no logins to list. %@ will be replaced with the application name") | |||
static let syncTimedOut = NSLocalizedString("sync.timeout", value: "Sync timed out", comment: "This is the message displayed when syncing entries from the server times out") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyone we need to inform of new/changed localized strings?
Shared/Store/BaseDataStore.swift
Outdated
return | ||
} | ||
|
||
queue.async { | ||
self.queue.asyncAfter(deadline: .now() + 20, execute: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this queues for 20 seconds from .now()
, correct? I ask because I am not super-familiar with the interface, and most other systems seem to use milliseconds.
Also, is it worth making 20
a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it queues for 20 seconds from now. Constant is a good idea!
Fixes #1025
Testing and Review Notes
Not sure the best way to test this other than Network Link Conditioner / being on a very slow connect.
To Do