Desktop: Resolves #8625: Show missing sync password warning and link to FAQ #8644
Conversation
packages/lib/components/shared/config/shouldShowMissingPasswordWarning.ts
Show resolved
Hide resolved
| onClick={openMissingPasswordFAQ} | ||
| style={theme.linkStyle} | ||
| > | ||
| {_('Why is my password missing?')} |
There was a problem hiding this comment.
Possible alternative:
| {_('Why is my password missing?')} | |
| {_('Help')} |
There was a problem hiding this comment.
Yes, just Help would be good. That's a temporary message that we'll keep just for the ARM64 transition, so we don't want to create too many new translatable strings for it
|
|
||
| // The FAQ section related to missing passwords is specific to MacOS/ARM -- only show it | ||
| // in that case. | ||
| const showMacInfoLink = shim.isMac(); |
There was a problem hiding this comment.
It may make sense to update this condition such that the warning is only shown on ARM 64 Macs. I only have access to an x86_64 Mac, however, so would be unable to test an ARM64-specific condition manually.
There was a problem hiding this comment.
Yes please enable it only for ARM64. I think you just need to check process.arch === 'arm64'
| const targetsExpectingPassword = [ | ||
| 'webdav', 'nextcloud', 'amazon_s3', 'joplinServer', 'joplinCloud', | ||
| ].map(name => SyncTargetRegistry.nameToId(name)); |
There was a problem hiding this comment.
Usually sync target properties go in the SyncTargetXxx.ts class so that it's easy to find them back all in the same place. In that case you'd create a new property in BaseSyncTarget, probably true by default since most sync targets require a password, and you'd override for those that don't.
| onClick={openMissingPasswordFAQ} | ||
| style={theme.linkStyle} | ||
| > | ||
| {_('Why is my password missing?')} |
There was a problem hiding this comment.
Yes, just Help would be good. That's a temporary message that we'll keep just for the ARM64 transition, so we don't want to create too many new translatable strings for it
|
|
||
| // The FAQ section related to missing passwords is specific to MacOS/ARM -- only show it | ||
| // in that case. | ||
| const showMacInfoLink = shim.isMac(); |
There was a problem hiding this comment.
Yes please enable it only for ARM64. I think you just need to check process.arch === 'arm64'
packages/lib/components/shared/config/shouldShowMissingPasswordWarning.ts
Show resolved
Hide resolved
|
|
||
| settingComps.push( | ||
| <p key='missing-password-warning' style={warningStyle}> | ||
| {_('Warning: Missing password.')} {showMacInfoLink ? macInfoLink : null} |
There was a problem hiding this comment.
Also please make sure there's a space between "missing password." and the macInfoLink
|
I've applied the changes suggested above. Currently, the missing password banner, |
No I think that was a good change, we should keep it for all platforms. The FAQ link was very specific to ARM64 so it's the only place we need a special case |
Summary
This pull request
1and2are done on all systems.3is only done on MacOS.Screenshots
Testing
'webdav', 'nextcloud', 'amazon_s3', 'joplinServer', 'joplinCloud'Note: This has only been tested on an x86_64 Mac with the Joplin Sever sync target.
Notes
Currently, this applies to the following sync targets
However, it is possible that there are valid reasons for some of these sync targets to have an empty password (e.g. a
localhostWebDAV server). Perhaps some of these sync targets should be removed from the list.Additionally, the missing password FAQ link is shown on any Mac.