Skip to content
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

Added PWA #2033

Merged
merged 1 commit into from
Apr 18, 2021
Merged

Added PWA #2033

merged 1 commit into from
Apr 18, 2021

Conversation

tonyvugithub
Copy link
Contributor

@tonyvugithub tonyvugithub commented Mar 26, 2021

Issue This PR Addresses

Fixed #801

Type of Change

  • Bugfix: Change which fixes an issue
  • New Feature: Change which adds functionality
  • Documentation Update: Change which improves documentation
  • UI: Change which improves UI

Description

This PR turns our Telescope into a Progressive Web App (PWA) to improve user experience on mobile.
Reference: https://github.com/vercel/next.js/tree/canary/examples/progressive-web-app

Choose Install app on Chrome


Checklist

  • Quality: This PR builds and passes our npm test and works locally
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not (if applicable)
  • Documentation: This PR includes updated/added documentation to user exposed functionality or configuration variables are added/changed or an explanation of why it does not(if applicable)

@tonyvugithub tonyvugithub marked this pull request as draft March 26, 2021 14:29
@tonyvugithub tonyvugithub self-assigned this Mar 26, 2021
@humphd humphd added the Blocked Can't do this, until something else is done label Mar 26, 2021
@humphd
Copy link
Contributor

humphd commented Mar 26, 2021

@tonyvugithub please add more details to this beyond "Ongoing process..."

@humphd
Copy link
Contributor

humphd commented Mar 26, 2021

If this is ready for review, remove Draft and let me know.

@tonyvugithub
Copy link
Contributor Author

@humphd : I am still adding some screenshots and it should be ready for reviewing

@tonyvugithub tonyvugithub marked this pull request as ready for review March 26, 2021 16:00
@tonyvugithub tonyvugithub removed the Blocked Can't do this, until something else is done label Mar 26, 2021
@chrispinkney
Copy link
Contributor

chrispinkney commented Mar 26, 2021

Wow, nice. I just tested the deployment on my phone (using Firefox) and it works really well! There are some issues (like logging in not working, or search being broken or about page 404ing) but the deployment had the same issues so this is not a PWA problem.

Here's some screenshots I took:



@tonyvugithub tonyvugithub force-pushed the issue-801 branch 2 times, most recently from a5942b4 to 7b8a08f Compare March 26, 2021 17:23
@tonyvugithub tonyvugithub marked this pull request as draft March 26, 2021 17:25
@tonyvugithub tonyvugithub added the Blocked Can't do this, until something else is done label Mar 26, 2021
@tonyvugithub
Copy link
Contributor Author

@humphd , @chrispinkney The conflict problem is bigger than I thought. So far none of my attempt has worked. Should we consider upgrade About to a component?

@humphd
Copy link
Contributor

humphd commented Mar 26, 2021

I feel like we're hitting a lot of bugs that relate to About, and not actually doing a ton with it. I'm fine to alter how it works for sure.

@tonyvugithub
Copy link
Contributor Author

@huynguyez : Actually it is the icon that has the square background https://github.com/Seneca-CDOT/telescope/blob/master/src/web/public/logoImages/manifest-icon-192.png.
If you want a rounded icon then @PedroFonsecaDEV needs to create one for me.

chrispinkney
chrispinkney previously approved these changes Apr 16, 2021
@PedroFonsecaDEV
Copy link
Contributor

@huynguyez
IMG_6953292F943A-1
The icon looks fine on my phone. We can investigate how to make it looks prettier on a Mac, but this is going to be in a follow-up.

Congrats @tonyvugithub, let's land this.

@humphd
Copy link
Contributor

humphd commented Apr 16, 2021

Based on conversation on Slack, we agreed that we are going to wait on the response from the next PWA maintainer about how the update path works. If things will be OK, and not require custom UI from us, this is probably good to land.

@chrispinkney chrispinkney dismissed their stale review April 16, 2021 18:10

see note from Dave

@tonyvugithub
Copy link
Contributor Author

tonyvugithub commented Apr 16, 2021

@humphd He should reply me next couple of days as he still actively maintains the repo.

@PedroFonsecaDEV PedroFonsecaDEV dismissed their stale review April 17, 2021 10:31

see note from Dave

@tonyvugithub
Copy link
Contributor Author

@PedroFonsecaDEV @chrispinkney @humphd : The next-pwa creator got back to me and he confirmed that if we changed prod it will update the cache in the PWA.
image

src/web/.babelrc Outdated Show resolved Hide resolved
src/web/next.config.js Outdated Show resolved Hide resolved
src/web/package.json Show resolved Hide resolved
src/web/public/manifest.json Show resolved Hide resolved
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can try this, and iterate on it later. One thing we could do that I notice is add caching for our posts api, so they get saved at runtime for offline access later.

src/web/package.json Show resolved Hide resolved
@tonyvugithub
Copy link
Contributor Author

@humphd That was sth I have in mind too as I know the image is cached but not the posts. However, it would require more research into the matter. I can always do a follow up in the future.

Copy link
Contributor

@chrispinkney chrispinkney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woohoo!

@tonyvugithub tonyvugithub merged commit 9d6de12 into Seneca-CDOT:master Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: PWA Related to Progressive Web App features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn Telescope into a Progressive Web App (PWA)
6 participants