spicygreenbook / greenbook-app

We are Spicy Green Book – a virtual directory for Black-owned food and beverage businesses throughout the US and Canada. We help Black-owned food and drink business owners tell their story, sowing the seeds of trust, support, and positive change within local communities.
https://www.spicygreenbook.org
46 stars 60 forks source link

Report: Mobile Speed #220

Open danilobatson opened 3 years ago

danilobatson commented 3 years ago

Website report states we can make improvements in our mobile speed. See image

image

ottograjeda commented 3 years ago

New Developer Volunteer here; i.e. 1st comment on this repo.

It’s not clear (to me) from the github label for this issue or comments in it, if this is a mobile_app or web_only thing. I double checked via PageSpeed for mobile devices & desktop... and both are low

~ 17 for mobile devices (the ~ is a tilde and is symbol for estimate) https://developers.google.com/speed/pagespeed/insights/?url=https%3A%2F%2Fspicygreenbook.org%2F

~ 29 for desktop https://developers.google.com/speed/pagespeed/insights/?url=https%3A%2F%2Fspicygreenbook.org%2F&tab=desktop

So I assume this is for web.

And checking github (this time issues/labels) again, I can’t determine 100% if someone self assigned this issue or not. So I will wait 24 hours or so, before looking into the issue, and if possible, improving the score. It won’t be a perfect 100 score for mobile devices or desktop but it should be a better score for both.

I will update this issue at that time.

pleaseshutup commented 3 years ago

This is for web.

We do static builds. The page could/would load instantly on web and score 100 on both, however, we have a hyrbrid app built on expo/nextjs and have to wait for the fonts to be ready. There is definitely an elegant solution here to get this to load instantaneously because we should have have to wait for data as it is injected into the build via server-side-rendering.

Some attempts were made but produced strange side effects (sorry to be vague, it was months ago when I last dug into this). You don't need to wait 24 hours. Fork the repo and have at it and good luck!

pleaseshutup commented 3 years ago

Forgot to add... This is mostly going to be done via nextjs. The ultimate way to test if everything is great is to do a static build.

You can test in dev via npx next dev

To do a build you do: npx next build

then npx next start to run the statically built site locally for doing a lighthouse test on it as you go.

ottograjeda commented 3 years ago

Ok ; I will work on the task and update the label (assign to me). And as FYI - PageSpeed reports more than just the font issue mentioned here.

ottograjeda commented 3 years ago

I did a quick clone, build, static export, & deploy to Google Firebase: here. And the scores are not much different: ~ 18 for mobile devices & ~ 35 for desktop.

PageSpeed/Lighthouse - both provide a detailed list of all issues and fonts, is just 1 of many. Images, about 60+ from Prismic, are not optimized, so that's one category with the most sub issues. Custom fonts, mentioned earlier, are only 7 for comparison. Who do I talk to about Prismic images ?

PS : The devsite is only temporary and has a robots disallow. I didn't have time today to setup a Vercel account and learn how to deploy to it. It's on my to_learn list for another day, for work on this issue.

02-04-2021 t 543 test_2

pleaseshutup commented 3 years ago

@ottograjeda prismic uses imgix to deliver images, the optimization is likely to provide either a casting to a certain type e.g. webp where needed but mostly to set the appropriate width for the device. This can be done via adding &w=500 as an example to the URL to the image. Check in ImageGallery.js you will see we do that but at a very basic level.

https://user-guides.prismic.io/en/articles/3309829-image-optimization-imgix-integration

https://github.com/spicygreenbook/greenbook-app/blob/master/components/ImageGallery.js#L149

ottograjeda commented 3 years ago

prismic uses imgix to deliver images, the optimization

?

Not sure why image compression is being done @ the tail end ; i.e. Client > Photographer > Prismic > Imgx > Developer. Usually this done @ the front, by the client/marketing_team who took the photo as part of the sign-off process so they... Client,Photographer,Marketing is 100% happy with it.

If not there, then by a Developer in the middle, who builds the app, prior to deployment, in bulk via custom script (say gulp) or package, with sign-off .... to avoid having N new open issues on a 1 photo/file/client basis (post deployment).

Usually I suggest to clients (and by extension to SGB), that images should be "src set" (by them/photog) & "lazy loaded" (by developer) using this. I even have a small/mini demo where it is used and it gets really good scores (see screen shot below).

...

What you are asking, if I read it correctly (I may not), is that the SGB process is : a developer edits 60+ LOC in N files manually, one at a time or via global search & replace with no approval/sign-off using Imgx ?

If so, the link provided is to the component - do you have a live sample where this is used so I can test it via PageSpeed/Lighthouse to see if it will work to get good scores ?

02-05-2021 src set and lazy loader sample

Disclaimer: I know you are the Tech Lead and I'm the newbie dev to the team, asking questions. Not trying to be a PITA, just double checking before I work on that.

ottograjeda commented 3 years ago

I didn't have time today to setup a Vercel account and learn how to deploy to it.

I made effort today to setup & deploy something via Next/Vercel. Kept notes & time, as I plan to help update the on boarding process for future devs per the new issue (post the slack discussion).

I still need to move the SGBapp into it before I make any edits to test (and close the previously created clone @ Google Firebase). It's just steady progress for now. All FYI.

... 02-05-2021 nextjs vercel 1

02-05-2021 nextjs vercel 2

02-05-2021 nextjs vercel 3

02-05-2021 nextjs vercel 4

pleaseshutup commented 3 years ago

And as FYI - PageSpeed reports more than just the font issue mentioned here. Yeap, there are a lot of nitpicks but this is the biggest render blocker from preventing the initial first render to be instant.

pleaseshutup commented 3 years ago

prismic uses imgix to deliver images, the optimization

?

Not sure why image compression is being done @ the tail end ; i.e. Client > Photographer > Prismic > Imgx > Developer. Usually this done @ the front, by the client/marketing_team who took the photo as part of the sign-off process so they... Client,Photographer,Marketing is 100% happy with it.

In the world of responsive design the photographer will simply provided the final edited images in a large format. It is on the devs to automatically resize the images on-the-fly depending on the device viewing them. A retina desktop display vs a tablet vs a small mobile screen as well as the actual placement on the page. Does it fill the full width of the device or partial? The photographer cannot know the crop and or different sizes needed ahead of time.

If not there, then by a Developer in the middle, who builds the app, prior to deployment, in bulk via custom script (say gulp) or package, with sign-off .... to avoid having N new open issues on a 1 photo/file/client basis (post deployment).

These images are hosted by the CDN and do not need to be part of the build process as we do not include them in the app nor host them with our front-end deployment.

Usually I suggest to clients (and by extension to SGB), that images should be "src set" (by them/photog) & "lazy loaded" (by developer) using this. I even have a small/mini demo where it is used and it gets really good scores (see screen shot below).

We can certainly use this technique and we don't need to provide different sizes via a build step since we can dynamically set them via the URL parameters of the image.

What you are asking, if I read it correctly (I may not), is that the SGB process is : a developer edits 60+ LOC in N files manually, one at a time or via global search & replace with no approval/sign-off using Imgx ?

I was not specifically suggesting a specific code edit, only providing an example. I would have to look into each issue on the lighthouse/pagespeed score to determine where to optimize and I have not looked into that level of detail yet. Simply providing some context and examples to help you as you dig into this.

If so, the link provided is to the component - do you have a live sample where this is used so I can test it via PageSpeed/Lighthouse to see if it will work to get good scores ?

Yes if you run the app locally via npx next dev and/or do a build and test the "built" version with the technique i suggsted further up this thread.

02-05-2021 src set and lazy loader sample

Disclaimer: I know you are the Tech Lead and I'm the newbie dev, asking questions. Not trying to be a PITA, just double checking before I work on that.

If you do this you will need to do a switch for using src set on the web and the expo/react-native method separately as I don't think that feature is available (but i havn't looked it up). If you find a good solution that is fast and addresses the actual speed issues and page speed reported issues then you have a great process for doing it on all of the pages and providing us some direction on how to handle images moving forward. The solution has to work on web and mobile device (native). You could even build a custom component that makes it handled for us easily where we just feed it the URL and it handles the switch for web/mobile etc.

ottograjeda commented 3 years ago

Since there is another issue open for on-boarding new devs, and to avoid having a 10+ page thread, I am going to summarize

I may still have additional questions, as I'm still a new volunteer (with little on-boarding docs) BUT I think the above is the current summary. And since I now have a Vercel account and have cloned the SGB repo, I think I can dig into the code to create & test a solution. Solution should be completed Friday evening or earlier/sooner.

If you think I missed something, feel free to add. Thx !

pleaseshutup commented 3 years ago

Hopefully this helps: With nextjs we use getStaticProps to get all of the data needed to render the home page (well, all pages) so that they can be statically created at build time. This means that all pages, even though they are built in react, CAN be built to be like a statically code html page that should display instantly. Whenever data is updated in prismic (the CMS) it triggers a deployment. So when we add/update/remove any data (testimonials, listings, press etc) the whole site can be statically built again. This means our entire site can be static (fast) AND up-to-date without requiring server side processing.

In my haste as the lone developer rebuilding the whole thing from scratch and trying to make good native mobile apps and web apps from the same code base I had ignored some lingering performance bottlenecks. This is the major problem to solve to get things SUPER FAST. After this problem is solved everything else is fairly simple.

in pages/* these are the initial endpoints that build each page. pages/index.js loads the <Main /> component.

The <Main /> Component is a wrapper for all of the logic and gets/sets state, fonts etc and then includes the screens/* files based on which screen you are rendering. Right now here is a hook isFrontEnd that is initially false and then a useEffect method turns it true. This forces the very first render to be <ActivityIndicator /> instead of the full site/content. In the past when we tried to render the full site there would be strange issues with layout/sizing that were beyond my comprehension. I could not tell if this was a bug in react-native/expo/react or we are just doing things wrong. So in a nutshell we need to be able to render the whole home page in the FIRST render loop properly without issues on web. If we solve this issue we're in a really good spot to optimize everything else from there.

C-Garza commented 3 years ago

Initial rendering issues could maybe mean that initial values are off when rendering the files on the server, like dimension sizes. That could be the cause of those layout issues, I do see some inline styles that are dependent on the dimension size. On the server, those values are probably set initially to undefined and conflict with the client side, and that's why a re-render fixes them.

ottograjeda commented 3 years ago

Here is the update

Post upgrade, I built & deployed the code here.

I didn't update all pages/screens as you will need to review the feature, as well as think about the pros & cons. Thus, I only tested on "FAQ" & "Team" . PageSpeed scores (per below graphics) marginally improved for mobile (still in red) and for desktop, appear better (now in orange).

Let me know how you want to proceed.

-- Otto

PS: In case github diff isn't great, main files touched/updated (in no order) are : package.json, next.config.js, public/, Footer.js, Nav.js, FAQ.js, & Team.js .

02-11-2021 3 sgb team - pagespeed

02-11-2021 3 sgb faq - pagespeed

pleaseshutup commented 3 years ago

Looks like an improvement. If the app works great on web and expo then you can make a PR and once merged we can begin to switch to using next/image where appropriate or possibly make our own wrapper for to use the proper switch for web/native or replace to do this for us.

Did you make any progress on the core issue described above? That's what's going to get us to 100.

pleaseshutup commented 3 years ago

I've also been tinkering with attempting to score higher: https://github.com/spicygreenbook/greenbook-app/tree/test1

ottograjeda commented 3 years ago

If the app works great on web and expo

It works on web ; anyone can go the URL posted earlier and test /team or /faq.

As for testing mobile, you want me to test via expo on desktop or online via a snack ? Am double checking before I make a PR and that gets kicked back because I tested wrong.

...

we can begin to switch to using next/image where appropriate...

The switch is easy ; it's just updating the files mentioned earlier; i.e.

PS: In case github diff isn't great, main files touched/updated (in no order) are : package.json, next.config.js, public/, Footer.js, Nav.js, FAQ.js, & Team.js .

...

Did you make any progress on the core issue described above? That's what's going to get us to 100.

IMHO, I believe the artifacts from 3rd party (next, vercel, imgix) will prevent a 100 score. However, scores (for all pages where next/image feature is used) will improved.

pleaseshutup commented 3 years ago

To test in expo you run expo start on your desktop terminal and then use the Expo Go app on your phone to open the provided test URL which loads the mobile app package on your phone to test on the mobile device.

pleaseshutup commented 3 years ago

@ottograjeda could you test in expo (check that native apps work on ios or android) and then do a PR? We need to get this update to nextjs v10 and use of next/image implemented ASAP to help improve the performance of things ASAP.

You have done great work so far and I do not want to have to do duplicate work to get this patched. Once we merge your changes into master we can start to optimize everything else as we go.

pleaseshutup commented 3 years ago

No update in 4 days i need to take this over to improve web performance per @danilobatson reporting it as "almost unusable" for web.

pleaseshutup commented 3 years ago

I've created <HybridImage /> and <HybridBackgroundImage /> as a drop-in replacement for <Image /> and <BackgroundImage /> we can use these to utilize https://nextjs.org/docs/api-reference/next/image in order to have it auto size images/lazy load etc to improve load times. We've also upgrade to nextjs v10 in package.json so please npm update when you start working on something or if you have done a fork you may wish to update that fork (edited)

Please check commit 95f4d99b49fac46ae1e508e092b9acce6d3ba1e0

danilobatson commented 3 years ago

Checking back in. If we are satisfied with improvements. Feel free to close @pleaseshutup @ottograjeda