Scryfall links/popups #107
Conversation
Fixes #94
There was a problem hiding this comment.
Wow nice! Great work. Couple blocking UX issues in addition to the comments:
- After mouseover but before the image loads, it's impossible to tell there's support for hover images. We should have some visual indication that something's happening, or people won't notice that an image would have appeared if they waited another couple of seconds.
- On mobile, the card images can get cut off on the side of the page (left side for me).
And a couple nice-to-haves, totally not blocking but we can at least make GitHub issues for them if they're not addressed here:
- On mobile it's hard to see the card images or know that we have them, since there's no mouseover. You can kind of see them by longpressing, but it doesn't feel supported and it's not a well-known behavior.
- Can we precache the card images? We should be able to pass a
staticFileGlobsoption into sw-precache, which generates our service worker that already precaches everything else on the site.
js/cardpopup.js
Outdated
| const queryURI = new URL( target.href ), | ||
| directURI = new URL( data.scryfall_uri ), | ||
| utmSource = queryURI.searchParams.get( 'utm_source' ); | ||
| directURI.searchParams.set( 'utm_source', utmSource ); |
There was a problem hiding this comment.
I'm having trouble finding any documentation about utm_source in the Scryfall API docs; do they ask consumers to set this? And if so can you include that reason as a comment here, with a link to where they do?
There was a problem hiding this comment.
It's not a written policy. @csuhta requests this on an individual basis.
There was a problem hiding this comment.
Hey there: I'm a member of the @scryfall team. Please set utm_source=whatsinstandard for any traffic you send our way. ❤️
There was a problem hiding this comment.
FYI: In a subsequent commit I just hardcoded the utm_source. (We should still set it manually on each homepage link in case clientside javascript is disabled.)
package.json
Outdated
| @@ -40,6 +40,7 @@ | |||
| "npm-font-open-sans": "^1.1.0", | |||
| "popper.js": "^1.14.3", | |||
| "sw-precache": "^5.2.1", | |||
| "tippy.js": "^2.6.0", | |||
There was a problem hiding this comment.
We're already using vue-tippy, is there a way we can reuse that instead of having two separate tippys included?
EDIT: Just saw your comment over on #94; imo we should definitely consolidate into one of the two tippys. Requiring both seems wasteful and liable to create complications down the road.
There was a problem hiding this comment.
Yeah, I'll wrestle with it until I figure out how it works.
There was a problem hiding this comment.
I removed the vanilla tippy.js dependency, although I'm still just using the tippy.js that's itself a dependency of VueTippy. I didn't "vue-ify" my code.
Remove unused eslint command. Hardcode utm_source for this site. Combine two calls into one.
Also removes vanilla Tippy
|
The vue-tippy dependencies we require is heavily outdated by the way. 0.2.8 vs. 2.0.19 |
| "vue-tippy": "^0.2.8" | ||
| "underscore": "^1.9.1", | ||
| "vue": "^2.5.17", | ||
| "vue-tippy": "^2.0.19" |
|
Responded to inline comments individually. W.r.t other comments:
Added progress cursor in 9ae81fe.
For me, at the moment, mobile is so nonfunctional (on iOS) that I can't get them to show. See next item.
See NilsEnevoldsen/ScryfallLinks#11. I worked on this last year, but I just don't know how to implement the behavior I want on mobile. Mobile browsers seemed finicky. Help appreciated.
Absolutely, but I don't know how. In fact, on a page like this, we don't need the client to fetch data from Scryfall at all; the appropriate HTML and blobs could all be delivered to the client by the WhatsInStandard server. I don't know how to "preprocess" that, short of requiring the human developer to enter it all manually. There's probably a "node" way to do it? |
|
Cool, this is in a good state to merge imo 👍 the part about requiring a human developer to do stuff for precaching is on point. So let's punt on precaching until we have bans in the API, which will make doing it programmatically easier. I can add some runtime caching myself later, that shouldn't block this branch because we never had it with Deckbox previews either, same for my comment about mobile. If you're curious in general about that stuff, look up service workers. You can actually turn off your internet and still load whatsinstandard.com from a new tab :P Thanks for your hard work! |
|
This works very well and looks gooood, thank you @NilsEnevoldsen! |
Fixes #94