feat: PWA pull to refresh #1599

Soxasora · 2024-11-17T21:12:37Z

Description

Pull-to-refresh for iOS, completing #1258.

As router.reload() is not graceful and router.replace(...) adds "/~" to the path breaking functionality across the app, I've decided to use router.push() instead, reloading the same page.
This updates the browser's history but since we're talking about a PWA there's no history to be accessed from the user.

This component wraps <Layout /> and is present in every page that make use of it.

Screenshots

doc_2024-11-17_22-03-08.mp4

Additional Context

At the moment it's enabled also for Android, disabling its native pull-to-refresh. If we want to enable it back we can pass android=false

Not being a master of creativity and CSS I want to know if you'd like to see something else instead of refreshing..., even though I think it's geeky in its own way

Checklist

Are your changes backwards compatible? Please answer below:
checkPWA prevents it from being triggered on anything that's not a PWA, also being a new component that has minimal CSS and loads only if the user is at the top of the page, I can say that it doesn't break functionality.

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
7, I don't have an Android phone and I would like to know if there are any problems with it, as said before we can disable or enable Android's PTR.
As it should work only in a PWA context, I also tested it on browsers

For frontend changes: Tested on mobile? Please answer below:
Yes, being a mobile-only feature

Did you introduce any new environment variables? If so, call them out explicitly here: No

ekzyis

Hey, this is very impressive! It works great and the code is very clean. Stackers (myself included) are going to love this! Additionally, this was a breeze to review. Thank you so much for this! Looking forward to more contributions from you 👀

Now to the review:

I tested this on desktop by simply manually setting isPWA to true and then using the mobile emulator. I also tested that the refresh actually works by loading new comments that I created in a new tab.

I only have a few UX and CSS nitpicks:

is it possible to prevent scrolling the window down when the pull to refresh shows up and I want to abort?

2024-11-18.01-26-46.mp4

Why did you add the spacer during refresh? Afaict it does nothing except create layout shift. Therefore I think we should also remove it, it's even more cleaner that way. So the meme should have actually been this:

the pull message is a little big so I think we should use font-size: small in the pullMessage CSS class.
~~the distance to pull down might be a little too much (almost half of the screen) but we can roll this out and then see if we need to adjust it.~~ actually, ignore this. I played around more and a big threshold as-is is better.
pulling down also works when I start to pull down from the bottom of the screen when I am scrolled all the way to the top. Not sure if that's how other apps do it, I think you need to start at the top?

After dropping refreshing ..., the spacer and making the text smaller, the UI/UX is this:

2024-11-18.01-53-19.mp4

Regarding Android: I haven't tested it on Android but since this works so well, I think we can keep your code enabled on Android to have a consistent experience.

If anything is weird, it's very easy to go back to the native behavior on Android.

Since these are all just nitpicks and it's already as-is a huge improvement, this is approved from my side ✅

_{@huumn is the final boss though}

ekzyis · 2024-11-18T00:21:43Z

components/pull-to-refresh.js

+  useEffect(() => {
+    checkPWA()
+  }, [])


Suggested change

useEffect(() => {

checkPWA()

}, [])

useEffect(checkPWA, [])

ekzyis · 2024-11-18T01:00:28Z

components/pull-to-refresh.js

+  }, [isPWA, isAndroid, android])
+
+  const handleTouchMove = useCallback((e) => {
+    if (touchStartY.current === 0) return


Is this needed so the text doesn't get stuck in some cases? At least that's what I can tell starts happening when I remove this line. A comment in the code would be nice here.

Yes! I thought I was going crazy because between refreshes the text would get stuck, noted thanks :P

ekzyis · 2024-11-18T01:03:20Z

components/pull-to-refresh.js

+  }, [])
+
+  const handleTouchEnd = useCallback(() => {
+    if (touchStartY.current === 0 || touchEndY.current === 0) return


Same reason here I suppose

Touch can be finicky with JS, I wanted to make sure that we can't encounter weird behavior

ekzyis · 2024-11-18T01:10:22Z

components/pull-to-refresh.js

+import { useState, useRef, useEffect, useCallback } from 'react'
+import styles from './pull-to-refresh.module.css'
+
+const PULL_THRESHOLD = 300


~~100 as twice the threshold when pull to refresh starts showing might be a better threshold.~~, No, I played around with 100 and 300 is way better since it doesn't cause accidental refreshes.

I would also call this REFRESH_THRESHOLD since PULL_THRESHOLD sounds like that's when we show the first message.

Noted, you're right it would make much more sense!

ekzyis · 2024-11-18T01:14:06Z

components/pull-to-refresh.js

+        {pullDistance > 0 || isRefreshing
+          ? (
+            <>
+              <p className={`${styles.pullMessage} ${pullDistance > 50 || isRefreshing ? styles.fadeIn : ''}`}>


This suggests you want to show the message when pullDistance > 50, but above you're checking for pullDistance > 0. Isn't checking it at one place enough? Using only pullDistance > 50 looks good to me.

Ok so, I think that stayed there during multiple versions of this code, IIRC before placing touch checks this was a workaround to get me through refreshes and avoid unintended messages.
I'm going to test if that's still the case

ekzyis · 2024-11-18T01:14:46Z

components/pull-to-refresh.js

+              <p className={`${styles.pullMessage} ${pullDistance > 50 || isRefreshing ? styles.fadeIn : ''}`}>
+                {getPullMessage()}
+              </p>
+              {isRefreshing && <div className={styles.spacer} />}


I think we should also drop this since afaict, it only causes layout shift

And it doesn't look clean

ekzyis · 2024-11-18T01:15:13Z

components/pull-to-refresh.module.css

+.spacer {
+    position: relative;
+    margin-bottom: 32px;
+}


see other comment

ekzyis · 2024-11-18T01:18:21Z

components/pull-to-refresh.module.css

+    left: 50%;
+    top: -50px;
+    transform: translateX(-50%);
+    font-size: 18px;


Suggested change

font-size: 18px;

font-size: small;

ekzyis · 2024-11-18T01:37:06Z

is it possible to prevent scrolling the window down when the pull to refresh shows up and I want to abort?

a solution for this might be to use the behavior in your first video where the whole page moves down when you pull down

Soxasora · 2024-11-18T08:26:03Z

Hey, this is very impressive! It works great and the code is very clean. Stackers (myself included) are going to love this! Additionally, this was a breeze to review. Thank you so much for this! Looking forward to more contributions from you 👀

Many many thanks! I'm glad that you like it!

is it possible to prevent scrolling the window down when the pull to refresh shows up and I want to abort?

a solution for this might be to use the behavior in your first video where the whole page moves down when you pull down

The behavior you're encountering should be uniquely present on desktop as it doesn't allow overscroll like mobile devices, if you notice in my two videos, the whole page moves down when you pull down :P

wait- now that I think about it, to make it theoretically work on Android I conditionally disable overscroll, we would get the same result you get on desktop... we definitely need to go with your UI/UX changes in order to make it pretty for both devices

Why did you add the spacer during refresh? Afaict it does nothing except create layout shift. Therefore I think we should also remove it, it's even more cleaner that way. So the meme should have actually been this:

The spacer prevents the messages to appear on content, but since we're going towards a much cleaner experience I think it won't be a problem anymore (thanks!)

the pull message is a little big so I think we should use font-size: small in the pullMessage CSS class.

I played around more and a big threshold as-is is better.

I agree wholeheartedly to both, about the threshold, I tried with lots of values and 300 was the right number imo!

pulling down also works when I start to pull down from the bottom of the screen when I am scrolled all the way to the top. Not sure if that's how other apps do it, I think you need to start at the top?

So as long as the user is at the top of the page, refresh can (or should) happen because you can't scroll higher, if the user is scrolling from the bottom but can't reach threshold it will make the user think that for scrolling he needs to start pulling from top.

After dropping refreshing ..., the spacer and making the text smaller, the UI/UX is this

You're right, I've been too conservative in terms of UI/UX

Regarding Android: I haven't tested it on Android but since this works so well, I think we can keep your code enabled on Android to have a consistent experience.

Thanks!!! Your changes will be beneficial to android too because of disabled overscroll :P

Since these are all just nitpicks and it's already as-is a huge improvement, this is approved from my side ✅

Nitpicks are what makes apps and sites feel good to use, thank you so much, will commit changes later in the day ❤️

Soxasora · 2024-11-18T14:54:38Z

trim.5FAC5AE4-5EC9-4B63-B42A-E4C4A1454145.MOV

well it looks super cool.

I moved overscroll-behavior-y into the body to avoid double refreshes, applied changes and corrected some typos

Soxasora · 2024-11-18T19:23:02Z

i forgot to memoize the messages 🫣
with calculated margin we can stay safe across devices, didn't cross my mind, thanks @huumn!

Soxasora added 2 commits November 17, 2024 20:23

feat: pull-to-refresh for PWA without native refresh

eea121e

test: togglable Android's native PTR

5a359fe

ekzyis self-requested a review November 17, 2024 23:52

ekzyis approved these changes Nov 18, 2024

View reviewed changes

enhance: cleanup, ux/ui changes, safer approach to android's ptr

a992426

Soxasora force-pushed the feat_pwa_pull_to_refresh branch from a4db44c to a992426 Compare November 18, 2024 14:50

add body margin on ptr

d2b5d23

huumn merged commit c3b7ad3 into stackernews:master Nov 18, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: PWA pull to refresh #1599

feat: PWA pull to refresh #1599

Soxasora commented Nov 17, 2024 •

edited

Loading

ekzyis left a comment •

edited

Loading

ekzyis Nov 18, 2024

Soxasora Nov 18, 2024

ekzyis Nov 18, 2024

Soxasora Nov 18, 2024

ekzyis Nov 18, 2024

Soxasora Nov 18, 2024

ekzyis Nov 18, 2024 •

edited

Loading

Soxasora Nov 18, 2024

ekzyis Nov 18, 2024 •

edited

Loading

Soxasora Nov 18, 2024 •

edited

Loading

ekzyis Nov 18, 2024

Soxasora Nov 18, 2024

ekzyis Nov 18, 2024

ekzyis Nov 18, 2024

ekzyis commented Nov 18, 2024

Soxasora commented Nov 18, 2024 •

edited

Loading

Soxasora commented Nov 18, 2024

Soxasora commented Nov 18, 2024

feat: PWA pull to refresh #1599

feat: PWA pull to refresh #1599

Conversation

Soxasora commented Nov 17, 2024 • edited Loading

Description

Screenshots

Additional Context

Checklist

ekzyis left a comment • 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

ekzyis Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekzyis Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

Soxasora Nov 18, 2024 • 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

ekzyis commented Nov 18, 2024

Soxasora commented Nov 18, 2024 • edited Loading

Soxasora commented Nov 18, 2024

Soxasora commented Nov 18, 2024

Soxasora commented Nov 17, 2024 •

edited

Loading

ekzyis left a comment •

edited

Loading

ekzyis Nov 18, 2024 •

edited

Loading

ekzyis Nov 18, 2024 •

edited

Loading

Soxasora Nov 18, 2024 •

edited

Loading

Soxasora commented Nov 18, 2024 •

edited

Loading