Adding back button & reload support #495
Conversation
| @@ -24,17 +24,20 @@ if (!self.define) { | |||
| if (registry[name]) { | |||
| return; | |||
| } | |||
|
|
|||
| const url = '/' + name; | |||
There was a problem hiding this comment.
Had to do this to prevent 404s, when it tried to request stuff like /game/index-abcdef.js
There was a problem hiding this comment.
Do you mean 'game/index-abcdef.js'?
| gameTypeToURL(width, height, mines, usedKeyboard) | ||
| ); | ||
| } | ||
| // /TODO |
There was a problem hiding this comment.
We can remove this once we're confident few people are upgrading from versions that used immedateGameSessionKey.
I'll create an issue for that.
|
Hmm, the rewrite rule is not working |
|
Hold off reviewing while I fix shit |
jakearchibald
force-pushed
the
back-btn-history
branch
from
76f9e19 to
926fb37
Compare
jakearchibald
force-pushed
the
localisation
branch
from
22cdc93 to
b4335e3
Compare
jakearchibald
force-pushed
the
back-btn-history
branch
from
4e35fb8 to
3c7b11c
Compare
There was a problem hiding this comment.
Yeah this looks good to me!
@PaulKinlan is gonna be so happy.
| @@ -24,17 +24,20 @@ if (!self.define) { | |||
| if (registry[name]) { | |||
| return; | |||
| } | |||
|
|
|||
| const url = '/' + name; | |||
There was a problem hiding this comment.
Do you mean 'game/index-abcdef.js'?
| return; | ||
| } | ||
|
|
||
| if (!location.pathname.startsWith("/game/")) { |
There was a problem hiding this comment.
I feel like the 2 if and the one inverted if are easy to trip over. Would a classic switch be more readable here?
const [,component] = location.pathname.split("/");
switch(component) {
case "": // start screen
// ...
break;
case "about":
// ...
break;
case "game":
// ...
break;
default:
this._resetToStartScreen()
}There was a problem hiding this comment.
Yeah I'll find a better pattern here
| if (!usedKeyboard) { | ||
| // This is a horrible hack to tell focus-visible.js not to initially show focus styles. | ||
| document.body.dispatchEvent( | ||
| new MouseEvent("mousemove", { bubbles: true }) |
There was a problem hiding this comment.
lol works for me :D (But also cc @robdodson)
There was a problem hiding this comment.
This code exists already, I've just moved it. WICG/focus-visible#198
There was a problem hiding this comment.
just checking, is there anything I need to do on my end?
There was a problem hiding this comment.
Oh no, sorry. I just wanted to make sure you are aware of this hack in case we were missing something. I didn’t know that Jake already filed an issue ages ago.
| if ( | ||
| url.pathname === "/" || | ||
| url.pathname === "/about/" || | ||
| url.pathname.startsWith("/game/") |
There was a problem hiding this comment.
I worry about having to keeping the pages the router knows about in sync with the pages the service worker knows about. We don't need to solve it in this PR, but maybe we should just switch to always delivering index.html on 404 for requests then end in /, .html or have no file extension?
There was a problem hiding this comment.
That won't work offline though (and has the lifi problem). I agree it's a problem though. I guess we should create a seperate file of rewrites that feeds into this and _redirects.
|
@jakearchibald is anything still left in this issue , which is not yet resolved ? |
I mean, I did leave comments that Jake wanted to address. |
|
The big "DON'T MERGE" thing is a clue. |
DON'T MERGE. This is based on the localisation work, so they'll need to land in that order (unless I untangle it, which isn't too hard).
Fixes #412.