-
Notifications
You must be signed in to change notification settings - Fork 0
Preload icons #177
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
base: main
Are you sure you want to change the base?
Preload icons #177
Changes from all commits
b9c7082
2eb1b7e
2d99080
ab14077
721154f
3dae519
caf9582
d09bbf1
712425e
9f03415
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,7 @@ | |
| * @category BIG | ||
| * @module App | ||
| */ | ||
| import React, { useState } from 'react' | ||
| import React, { useEffect, useState, useRef } from 'react' | ||
| import 'react-native-gesture-handler' | ||
| import AppLoading from 'expo-app-loading' | ||
| import { Provider } from '@nationskollen/sdk' | ||
|
|
@@ -13,33 +13,78 @@ import { LanguageContextProvider } from './translate/LanguageContext' | |
| import { setCustomText, setCustomTextInput } from 'react-native-global-props' | ||
| import { DarkTheme, LightTheme, ThemeProvider, Theme } from './components/ThemeContext' | ||
| import { useFonts, Roboto_700Bold, Roboto_400Regular } from '@expo-google-fonts/roboto' | ||
|
|
||
| import { Ionicons, MaterialCommunityIcons, MaterialIcons } from '@expo/vector-icons' | ||
| import * as Font from 'expo-font' | ||
| import Constants from 'expo-constants' | ||
| import * as Notifications from 'expo-notifications' | ||
| import Footer from './components/Footer/Footer' | ||
| import { Platform } from 'react-native' | ||
|
|
||
| Notifications.setNotificationHandler({ | ||
| handleNotification: async () => ({ | ||
| shouldShowAlert: true, | ||
| shouldPlaySound: false, | ||
| shouldSetBadge: false, | ||
| }), | ||
| }) | ||
|
|
||
| const App = () => { | ||
| const [initialTheme, setInitialTheme] = useState<Theme | null>(null) | ||
| const [initialLanguageKey, setInitialLanguageKey] = useState<number>(1) | ||
| const [isReady, setIsReady] = useState(false) | ||
| const [expoPushToken, setExpoPushToken] = useState('') | ||
| const [notification, setNotification] = useState<any>(false) | ||
|
|
||
| const responseListener = useRef<any>() | ||
| const notificationListener = useRef<any>() | ||
|
|
||
| const [loaded] = useFonts({ | ||
| Roboto_400Regular, | ||
| Roboto_700Bold, | ||
| }) | ||
|
|
||
| // We have to to wait for the app to load the custom font before we render it | ||
| if (!loaded) { | ||
| return null | ||
| } | ||
|
|
||
| /** Global font to be used across the app. */ | ||
| const customTextProps = { | ||
| style: { | ||
| fontFamily: 'Roboto_400Regular', | ||
| }, | ||
| } | ||
|
|
||
| /** Sets all text and inputboxes-fonts to the font | ||
| * set in customTextProps.*/ | ||
| setCustomText(customTextProps) | ||
| setCustomTextInput(customTextProps) | ||
|
|
||
| /** Preloads the fonts before app rendering. */ | ||
| function cacheFonts(fonts: { [x: string]: any }) { | ||
| return fonts.map((font: string) => Font.loadAsync(font)) | ||
| } | ||
|
|
||
| useEffect(() => { | ||
| registerForPushNotificationsAsync().then((token) => setExpoPushToken(token)) | ||
|
|
||
| notificationListener.current = Notifications.addNotificationReceivedListener( | ||
| (notification) => { | ||
| setNotification(notification) | ||
| } | ||
| ) | ||
|
|
||
| responseListener.current = Notifications.addNotificationResponseReceivedListener( | ||
| (response) => { | ||
| console.log(response) | ||
| } | ||
| ) | ||
|
|
||
| return () => { | ||
| Notifications.removeNotificationSubscription(notificationListener.current) | ||
| Notifications.removeNotificationSubscription(responseListener.current) | ||
| } | ||
| }) | ||
|
|
||
| if (!isReady) { | ||
| return ( | ||
| <AppLoading | ||
|
|
@@ -58,6 +103,17 @@ const App = () => { | |
| if (language) { | ||
| setInitialLanguageKey(parseInt(language)) | ||
| } | ||
|
|
||
| // Preload Icons | ||
|
|
||
| const fontAssets = cacheFonts([ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should also consider only using one icon pack. Loading three different icon packs seems inefficient (this can be done in a future in a separate PR, unless it is an easy fix, of course) |
||
| Ionicons.font, | ||
| MaterialCommunityIcons.font, | ||
| MaterialIcons.font, | ||
| ]) | ||
| await Promise.all([...fontAssets]) | ||
|
|
||
| registerForPushNotificationsAsync() | ||
| }} | ||
| onFinish={() => setIsReady(true)} | ||
| autoHideSplash={true} | ||
|
|
@@ -77,7 +133,7 @@ const App = () => { | |
| > | ||
| <ThemeProvider initialTheme={initialTheme}> | ||
| <LanguageContextProvider initialLanguage={initialLanguageKey}> | ||
| <PushTokenProvider> | ||
| <PushTokenProvider token={expoPushToken} notification={notification}> | ||
| <Footer /> | ||
| </PushTokenProvider> | ||
| </LanguageContextProvider> | ||
|
|
@@ -86,4 +142,39 @@ const App = () => { | |
| ) | ||
| } | ||
|
|
||
| async function registerForPushNotificationsAsync() { | ||
| let token: string | ||
|
|
||
| if (Constants.isDevice) { | ||
| const { status: existingStatus } = await Notifications.getPermissionsAsync() | ||
| let finalStatus = existingStatus | ||
|
|
||
| if (existingStatus !== 'granted') { | ||
| const { status } = await Notifications.requestPermissionsAsync() | ||
| finalStatus = status | ||
| } | ||
|
|
||
| if (finalStatus !== 'granted') { | ||
| console.log('Failed to get push token for push notification!') | ||
| return | ||
| } | ||
|
|
||
| token = (await Notifications.getExpoPushTokenAsync()).data | ||
| console.log(token) | ||
| } else { | ||
| console.log('Must use physical device for Push Notifications') | ||
| } | ||
|
|
||
| // TODO: Set options to match branding | ||
| if (Platform.OS === 'android') { | ||
| Notifications.setNotificationChannelAsync('default', { | ||
| name: 'default', | ||
| importance: Notifications.AndroidImportance.MAX, | ||
| vibrationPattern: [0, 250, 250, 250], | ||
| lightColor: '#FF231F7C', | ||
| }) | ||
| } | ||
| return token | ||
| } | ||
|
|
||
| export default App | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ export type BadgeProps = { | |
| export const BadgedIcon = ({ name, showNum, color }: BadgeProps) => { | ||
| const { colors } = useTheme() | ||
| const { token } = usePushToken() | ||
| const { data } = useNotifications(token) | ||
|
|
||
| const Icon = <Ionicons name={name} size={23} color={color ?? colors.text}></Ionicons> | ||
|
|
||
|
|
@@ -36,13 +37,11 @@ export const BadgedIcon = ({ name, showNum, color }: BadgeProps) => { | |
| return Icon | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check is not necessary now I believe, the Icon is returned regardless
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reverted the length check |
||
| } | ||
|
|
||
| const { data } = useNotifications(token) | ||
|
|
||
| return ( | ||
| <> | ||
| {Icon} | ||
|
|
||
| {data.length > 0 && ( | ||
| {data && ( | ||
| <Badge | ||
| value={showNum ? data.length : null} | ||
| containerStyle={styles.container} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,110 +5,46 @@ | |
| * @module PushTokenContext | ||
| */ | ||
| import React, { createContext, useCallback, useState, useContext, useRef, useEffect } from 'react' | ||
| import Constants from 'expo-constants' | ||
| import { Platform } from 'react-native' | ||
| import { useAsync } from 'react-async-hook' | ||
| import * as Notifications from 'expo-notifications' | ||
| import AsyncStorage from '@react-native-async-storage/async-storage' | ||
|
|
||
| export interface PushTokenContextContract { | ||
| token: string | ||
| lastUpdated?: Date | ||
| setLastUpdated: (date: Date) => void | ||
| notification: boolean | ||
| } | ||
|
|
||
| export interface Props { | ||
| children: Element | Element[] | ||
| expoToken: string | ||
| notification: any | ||
| } | ||
|
|
||
| export const PushTokenContext = createContext({} as PushTokenContextContract) | ||
| export const usePushToken = () => useContext(PushTokenContext) | ||
|
|
||
| Notifications.setNotificationHandler({ | ||
| handleNotification: async () => ({ | ||
| shouldShowAlert: true, | ||
| shouldPlaySound: false, | ||
| shouldSetBadge: false, | ||
| }), | ||
| }) | ||
|
|
||
| async function registerForPushNotificationsAsync() { | ||
| let token: string | ||
|
|
||
| if (Constants.isDevice) { | ||
| const { status: existingStatus } = await Notifications.getPermissionsAsync() | ||
| let finalStatus = existingStatus | ||
|
|
||
| if (existingStatus !== 'granted') { | ||
| const { status } = await Notifications.requestPermissionsAsync() | ||
| finalStatus = status | ||
| } | ||
|
|
||
| if (finalStatus !== 'granted') { | ||
| console.log('Failed to get push token for push notification!') | ||
| return | ||
| } | ||
|
|
||
| token = (await Notifications.getExpoPushTokenAsync()).data | ||
| console.log(token) | ||
| } else { | ||
| console.log('Must use physical device for Push Notifications') | ||
| } | ||
|
|
||
| // TODO: Set options to match branding | ||
| if (Platform.OS === 'android') { | ||
| Notifications.setNotificationChannelAsync('default', { | ||
| name: 'default', | ||
| importance: Notifications.AndroidImportance.MAX, | ||
| vibrationPattern: [0, 250, 250, 250], | ||
| lightColor: '#FF231F7C', | ||
| }) | ||
| } | ||
|
|
||
| return token | ||
| } | ||
|
|
||
| // TODO: Allow registration of callbacks for new notifications? | ||
| export const PushTokenProvider = ({ children }: Props) => { | ||
| const responseListener = useRef<any>() | ||
| const notificationListener = useRef<any>() | ||
| const [token, setToken] = useState<string | null>(null) | ||
| export const PushTokenProvider = ({ token, notification, children }) => { | ||
| const { result } = useAsync(async () => await AsyncStorage.getItem('lastUpdated'), []) | ||
| const setLastUpdated = useCallback( | ||
| (date: Date) => AsyncStorage.setItem('lastUpdated', date.toISOString()), | ||
| [] | ||
| ) | ||
|
|
||
| useEffect(() => { | ||
| registerForPushNotificationsAsync().then((token) => setToken(token)) | ||
|
|
||
| notificationListener.current = Notifications.addNotificationReceivedListener( | ||
| (notification) => { | ||
| console.log(notification) | ||
| } | ||
| ) | ||
|
|
||
| responseListener.current = Notifications.addNotificationResponseReceivedListener( | ||
| (response) => { | ||
| console.log(response) | ||
| } | ||
| ) | ||
|
|
||
| return () => { | ||
| Notifications.removeNotificationSubscription(notificationListener.current) | ||
| Notifications.removeNotificationSubscription(responseListener.current) | ||
| } | ||
| }, []) | ||
|
|
||
| return ( | ||
| <PushTokenContext.Provider | ||
| value={{ | ||
| token, | ||
| lastUpdated: result ? new Date(result) : undefined, | ||
| setLastUpdated, | ||
| }} | ||
| > | ||
| {children} | ||
| </PushTokenContext.Provider> | ||
| <> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will prevent rendering of the app when the token is not defined (?), and this is not the the behaviour that we want. I think it is better to allow this to be null and instead check this in the components that actually use the token. For example, in the notification badge you can add a check after extracting the token from the context and check whether it is null or not. If that is the case, simply return null and skip the rendering of the component. That should work, as long as the check happens before the
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would mean that the useNotifications hook is depentent on if-statement, why would that not be illegal?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, you are right, it would be illegal. I thought it was okay to do this as long as the ordering of hooks remained the same. Apparently, this is not the case. I think it might work, but it is not actually allowed and might cause bugs :) I suppose the solution would be to move the actual rendering of the count to a separate component? Good catch 👍🏼
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can look some more at this one.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Check latest commit, it sets a loading state in the async function, and returns the components once the functions loading state is false. It ensures that the component render, even if the token might be null, and at the same time ensures that the component doesn't render before the fetch is completed. This removes the previous API-errors of trying to fetch a null token, and react doesn't complain on illegal hook usage.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It leads though to a 500ms longer start of the app due to a token being forced to finish fetching before render, but that might be fixed with a longer splash screen. An (mabye ugly) fix for this would be to call useToken() in App.tsx so to set the token state, and when it's finished we then render the app. Using this method would then make the latest commit unnessecary.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having a slightly longer startup time is okay I guess. Since the app is actually running, we should be able to display some sort of loading indicator while the token is being fetched. |
||
| { | ||
| <PushTokenContext.Provider | ||
| value={{ | ||
| token, | ||
| lastUpdated: result ? new Date(result) : undefined, | ||
| setLastUpdated, | ||
| notification, | ||
| }} | ||
| > | ||
| {children} | ||
| </PushTokenContext.Provider> | ||
| } | ||
| </> | ||
| ) | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Should
fontsreally be of this type? Since you are usingmapI would assume it is an array of strings (Array<string>)?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.
Expos own type definition is
[x: string]: anyforIcon.font, so if i would changecacheFontstoArray<String>fontAssetscomplains about the type :/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.
Hm, that seems weird, especially since you pass in an array of fonts to the function on line 69?
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.
Yeah i know, i believe expo forced this type for some reason.