Preload icons #177

RobertMartinis · 2021-06-17T17:46:37Z

This PR preloads the icons of the homepage during the splash screen.
Also fixes a bug where the badge in notifications renders before the api-async is finished.

This action runs the formatter for us if we happen to forget to run it before pushing to main or PR. * Update main.yml * Formatted code Co-authored-by: McFrappe <[email protected]>

…cons

Frewacom · 2021-06-18T07:05:57Z

App.tsx

@@ -40,6 +41,10 @@ const App = () => {
    setCustomText(customTextProps)
    setCustomTextInput(customTextProps)

+    function cacheFonts(fonts: { [x: string]: any }) {


Should fonts really be of this type? Since you are using map I would assume it is an array of strings (Array<string>)?

Expos own type definition is [x: string]: any for Icon.font, so if i would change cacheFonts to Array<String> fontAssets complains about the type :/

Hm, that seems weird, especially since you pass in an array of fonts to the function on line 69?

Yeah i know, i believe expo forced this type for some reason.

Frewacom · 2021-06-18T07:09:35Z

App.tsx

+
+                    // Preload Icons
+
+                    const fontAssets = cacheFonts([


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)

Frewacom · 2021-06-18T07:11:42Z

components/Common/NotificationBadge.tsx

    return (
        <>
            {Icon}

-            {data.length > 0 && (
+            {data != undefined && (


Would it not be better to have both of these conditions set to true before rendering?

Suggested change

{data != undefined && (

{data && data.length > 0 && (

Yes, i forgot i removed the data.length.

Frewacom · 2021-06-18T07:18:21Z

components/PushTokenContext.tsx

-        >
-            {children}
-        </PushTokenContext.Provider>
+        <>


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 useNotifications hook.

That would mean that the useNotifications hook is depentent on if-statement, why would that not be illegal?

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 👍🏼

I can look some more at this one.

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 👍🏼

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.

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.

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.

johanneslil · 2021-06-18T09:03:54Z

components/Common/NotificationBadge.tsx

@@ -36,13 +37,11 @@ export const BadgedIcon = ({ name, showNum, color }: BadgeProps) => {
        return Icon


This check is not necessary now I believe, the Icon is returned regardless

Reverted the length check

Frewacom · 2021-06-20T08:57:25Z

components/PushTokenContext.tsx

@@ -32,7 +32,7 @@ Notifications.setNotificationHandler({
    }),
 })

-async function registerForPushNotificationsAsync() {
+async function registerForPushNotificationsAsync(setLoading) {


Would it not be better to call setLoading in the then(() => ...) callback instead (line 84)? Then you do not need any extra arguments and you will update all the state at the same place.

Frewacom · 2021-06-20T09:00:20Z

components/PushTokenContext.tsx

@@ -98,17 +99,21 @@ export const PushTokenProvider = ({ children }: Props) => {
            Notifications.removeNotificationSubscription(notificationListener.current)
            Notifications.removeNotificationSubscription(responseListener.current)
        }
-    }, [])
+    }, [token])


Is this effect-hook dependency really needed? The actual token is fetched inside the effect-hook and running the effect when the token changes seems weird (why would we fetch the token if we already have a token set?).

Running this effect on mount only ([]) seems more appropriate imo.

Yes, i dont know why this is here and why i added it. Will revert.

Frewacom · 2021-06-20T09:02:44Z

components/PushTokenContext.tsx

-        >
-            {children}
-        </PushTokenContext.Provider>
+        <>


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.

Frewacom · 2021-06-20T09:03:07Z

components/PushTokenContext.tsx

-            {children}
-        </PushTokenContext.Provider>
+        <>
+            {!loading && (


Add a loading indicator when loading

Good idea. Will add.

McFrappe and others added 6 commits June 17, 2021 16:12

* Autoformatter for when creating PRs and push

b9c7082

This action runs the formatter for us if we happen to forget to run it before pushing to main or PR. * Update main.yml * Formatted code Co-authored-by: McFrappe <[email protected]>

Merge branch 'main' of github.com:nationskollen/mobile into preload-i…

2eb1b7e

…cons

Preloading of icons

2d99080

Fix async error with provider

ab14077

Formatting

721154f

remove unused file

3dae519

Frewacom requested changes Jun 18, 2021

View reviewed changes

johanneslil reviewed Jun 18, 2021

View reviewed changes

RobertMartinis and others added 2 commits June 18, 2021 22:40

Add loading state to PushTokenContext

caf9582

Formatted code

d09bbf1

Frewacom requested changes Jun 20, 2021

View reviewed changes

RobertMartinis and others added 2 commits July 29, 2021 16:07

Add fetching push-token to App.tsx

712425e

Formatted code

9f03415

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preload icons #177

Preload icons #177

RobertMartinis commented Jun 17, 2021

Frewacom Jun 18, 2021

RobertMartinis Jun 18, 2021

Frewacom Jun 18, 2021

RobertMartinis Jun 18, 2021

Frewacom Jun 18, 2021 •

edited

Loading

Frewacom Jun 18, 2021

RobertMartinis Jun 18, 2021

Frewacom Jun 18, 2021 •

edited

Loading

johanneslil Jun 18, 2021

Frewacom Jun 18, 2021

RobertMartinis Jun 18, 2021

RobertMartinis Jun 18, 2021 •

edited

Loading

RobertMartinis Jun 18, 2021 •

edited

Loading

Frewacom Jun 20, 2021

johanneslil Jun 18, 2021

RobertMartinis Jun 18, 2021

Frewacom Jun 20, 2021

Frewacom Jun 20, 2021

RobertMartinis Jun 20, 2021

Frewacom Jun 20, 2021

Frewacom Jun 20, 2021

RobertMartinis Jun 20, 2021

		@@ -36,13 +37,11 @@ export const BadgedIcon = ({ name, showNum, color }: BadgeProps) => {
		return Icon

Preload icons #177

Are you sure you want to change the base?

Preload icons #177

Conversation

RobertMartinis commented Jun 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frewacom Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frewacom Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertMartinis Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

RobertMartinis Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frewacom Jun 18, 2021 •

edited

Loading

Frewacom Jun 18, 2021 •

edited

Loading

RobertMartinis Jun 18, 2021 •

edited

Loading

RobertMartinis Jun 18, 2021 •

edited

Loading