-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Desktop, Mobile: Add Joplin Cloud account information to configuration screen #10553
Merged
laurent22
merged 19 commits into
laurent22:dev
from
pedr:add-joplin-cloud-info-to-config
Jun 12, 2024
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
b57f1dc
add account information to joplin cloud screen
pedr 27a1354
moving accountTypeToString out of server package
pedr 1f33fdd
add joplin cloud acount info to mobile
pedr cec869c
moving function to proper file
pedr 8142462
using proper bridge import
pedr 420bd56
adding Team to account type
pedr 583abd0
add exhaustive check
pedr 4876d93
remove key
pedr 33b5162
Merge remote-tracking branch 'upstream/dev' into add-joplin-cloud-inf…
pedr 9ab7db2
spliting joplinCloud utils and fixing interface
pedr 98a2ce5
changing testing value to real one
pedr d8df6d1
add key to component
pedr d171a32
use unknown instead of default as fallback
pedr a638777
removing divider style
pedr bcfe23a
adding useCallback
pedr 586291a
fix import on desktop
pedr 7911be8
changing margin calculation to joplin var
pedr 584495b
renaming file to be more generic
pedr 271e08f
removing labels from config search
pedr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
87 changes: 87 additions & 0 deletions
87
packages/app-mobile/components/screens/ConfigScreen/JoplinCloudConfig.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| import { _ } from '@joplin/lib/locale'; | ||
| import * as React from 'react'; | ||
| import { useCallback } from 'react'; | ||
| import { View, Text, Linking } from 'react-native'; | ||
| import Clipboard from '@react-native-clipboard/clipboard'; | ||
| import SettingsButton from './SettingsButton'; | ||
| import { accountTypeToString } from '@joplin/lib/utils/joplinCloud/types'; | ||
| import { LinkButton } from '../../buttons'; | ||
| import { ConfigScreenStyles } from './configScreenStyles'; | ||
| import { Divider } from 'react-native-paper'; | ||
| import Logger from '@joplin/utils/Logger'; | ||
|
|
||
| const logger = Logger.create('JoplinCloudConfig'); | ||
|
|
||
| type JoplinCloudConfigProps = { | ||
| styles: ConfigScreenStyles; | ||
| accountType: string; | ||
| inboxEmail: string; | ||
| website: string; | ||
| userEmail: string; | ||
| }; | ||
|
|
||
| export const emailToNoteLabel = () => _('Email to note'); | ||
| export const emailToNoteDescription = () => _('Any email sent to this address will be converted into a note and added to your collection. The note will be saved into the Inbox notebook'); | ||
|
|
||
| const JoplinCloudConfig = (props: JoplinCloudConfigProps) => { | ||
|
|
||
| const isEmailToNoteAvailableInAccount = props.accountType !== '1'; | ||
| const inboxEmailValue = props.inboxEmail ?? '-'; | ||
|
|
||
| const goToJoplinCloudProfile = useCallback(async () => { | ||
| await Linking.openURL(`${props.website}/users/me`); | ||
| }, [props.website]); | ||
|
|
||
| const accountTypeName = () => { | ||
| try { | ||
| if (!props.accountType) return 'Unknown'; | ||
| return accountTypeToString(parseInt(props.accountType, 10)); | ||
| } catch (error) { | ||
| logger.error(error); | ||
| return 'Unknown'; | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <View> | ||
| <View style={props.styles.styleSheet.settingContainerNoBottomBorder}> | ||
| <Text style={props.styles.styleSheet.settingTextEmphasis}>{_('Account information')}</Text> | ||
| </View> | ||
| <View style={props.styles.styleSheet.settingContainerNoBottomBorder}> | ||
| <Text style={props.styles.styleSheet.settingText}>{_('Account type')}</Text> | ||
| <Text style={props.styles.styleSheet.settingTextEmphasis}>{accountTypeName()}</Text> | ||
| </View> | ||
| <View style={props.styles.styleSheet.settingContainerNoBottomBorder}> | ||
| <Text style={props.styles.styleSheet.settingText}>{_('Email')}</Text> | ||
| <Text selectable style={props.styles.styleSheet.settingTextEmphasis}>{props.userEmail}</Text> | ||
| </View> | ||
| <LinkButton onPress={goToJoplinCloudProfile}> | ||
| {_('Go to Joplin Cloud profile')} | ||
| </LinkButton> | ||
| <Divider bold /> | ||
|
|
||
| <View style={props.styles.styleSheet.settingContainerNoBottomBorder}> | ||
| <Text style={props.styles.styleSheet.settingText}>{emailToNoteLabel()}</Text> | ||
| <Text style={props.styles.styleSheet.settingTextEmphasis}>{inboxEmailValue}</Text> | ||
| </View> | ||
| { | ||
| !isEmailToNoteAvailableInAccount && ( | ||
| <View style={props.styles.styleSheet.settingContainerNoBottomBorder}> | ||
| <Text style={props.styles.styleSheet.descriptionAlert}>{_('Your account doesn\'t have access to this feature')}</Text> | ||
| </View> | ||
| ) | ||
| } | ||
| <SettingsButton | ||
| title={_('Copy to clipboard')} | ||
| clickHandler={() => isEmailToNoteAvailableInAccount && Clipboard.setString(props.inboxEmail)} | ||
| description={emailToNoteDescription()} | ||
| statusComponent={undefined} | ||
| styles={props.styles} | ||
| disabled={!isEmailToNoteAvailableInAccount} | ||
| /> | ||
| </View> | ||
| ); | ||
|
|
||
| }; | ||
|
|
||
| export default JoplinCloudConfig; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 changes: 2 additions & 2 deletions
4
packages/lib/utils/joplinCloud.ts → packages/lib/utils/joplinCloud/index.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| enum AccountType { | ||
| Default = 0, | ||
| Basic = 1, | ||
| Pro = 2, | ||
| Team = 3, | ||
| } | ||
|
|
||
| // eslint-disable-next-line import/prefer-default-export | ||
| export function accountTypeToString(accountType: AccountType): string { | ||
| if (accountType === AccountType.Default) return 'Default'; | ||
| if (accountType === AccountType.Basic) return 'Basic'; | ||
| if (accountType === AccountType.Pro) return 'Pro'; | ||
| if (accountType === AccountType.Team) return 'Team'; | ||
| const exhaustivenessCheck: never = accountType; | ||
| throw new Error(`Invalid type: ${exhaustivenessCheck}`); | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
We should probably default to using
useCallbackfor event handlers, especially when it's dependent on a prop like here. Probably the prop won't change but if for some reason it's initially empty, then set to a value, the component will incorrectly use the empty valueThere 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'm not sure that's true... I think
useCallbackprevents unnecessary re-renders of child components (rather than forcing the function to reload). More specifically, suppose thatprops.joplinCloudWebsitechanges from""to"https://joplincloud.com/":props.joplinCloudWebsiteis initially"". This is the initial render.goToJoplinCloudProfileis set to a function.props.joplinCloudWebsiteis""in its scope. As such, ifgoToJoplinCloudProfileis called, it willopenExternal('/users/me').Buttonis givengoToJoplinCloudProfileas a prop. This is the first render of thatButton.props.joplinCloudWebsitechanges to"https://joplincloud.com/". This causes the component to re-render.goToJoplinCloudProfileis set to a function.props.joplinCloudWebsiteis"https://joplincloud.com/"in its scope. As such, ifgoToJoplinCloudProfileis called, it willopenExternal('https://joplincloud.com//users/me').Buttonis givengoToJoplinCloudProfileas a prop. This prop is different from its previous value (becausefunction(){} !== function(){}). As a result, theButtonre-renders.goToJoplinCloudProfile.Observe that in step 2, because React considers different functions to be unequal, the
Buttonuses the new version of the function that includes"https://joplincloud.com/"forprops.joplinCloudWebsitein its scope.Also see react.dev's "Should you add useCallback everywhere".
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.
Ah yes, that's true, it will re-render anyway.
But I actually think that it's reasonable to default to adding useCallback rather than wondering whether it needs to be optimised or not. In this case maybe it doesn't make a difference that it re-render even when the prop doesn't change, but maybe it does, who knows. That's the idea of mostly defaulting to using useCallback - then we don't need to wonder about this, and it feels like it's more future proof