The basics are working great! There's a few known missing things though:
- Add reasonable noscript behavior
- Disable options where there's no valid appearance
- Lay it out actually _good_, instead of just images dumped there
Adapting what the Impress 2020 UI does, but in Ruby instead!
I feel like this is case is really starting to show the power of doing
this stuff in Rails instead of via an API… we can *really* take
advantage of our models and our handy idioms at all points. This is
just so much less *code* than this feature takes in Node + GraphQL +
React.
We had this issue on Impress 2020 and I fixed it over there too. I guess
it went less noticed here on Classic, because it's a more
progressively-enhanced site in general (and this failure case is an
interesting argument for that architecture! lol).
On Impress 2020, I wasn't sure if the "waits for the document to load"
behavior of the `defer` attribute was necessary to the script, so I
chose to keep `defer` but move it _after_ the other scripts.
This time, I dug in a bit more, and found a Plausible author saying
that the choice was kinda arbitrary; and another person who had the
same issue as me, who said they switched to `async` and it worked well.
So, that's what we're doing now, too!
https://github.com/plausible/analytics/discussions/1907#discussioncomment-2754499
Closes#2, after making some tweaks to the PR to fit how JS templating
works here. Thanks @dice!!
I had to move `petThumbnailUrl` out of the closure, because this script
does a cute thing of having separate variable scopes for the separate
areas of the page—but this is used by two of them. Arguably it could
make sense to like, put this all in one larger shared IIFE closure that
wraps both of them, to preserve some of this code's intention of
avoiding adding to the global namespace on this page, but like.
*It's fine.*
Co-Authored-By: Steve C <diceroll123@gmail.com>
We were previously planning a more interesting "Add to Cart"
integration with TNT, but it hasn't panned out! For now, we'll just
link to the NC Mall homepage.
Two reasons for this new title:
1. The pitch for "Get these items!" is weaker, now that we're not
getting the power-user integrations we'd planned around.
2. I literally only just thought of it now!
This doesn't do a good job maintaining state across morphs, but hey
it's Working At All in terms of wiring, and that's good!!
Also need to style up the toggle as a cute button instead of a visible
checkbox and the words "Play/pause"!
Add some unobtrusive white background for contrast, show it when the
Turbo frame is loading too, add a spinner cursor, and fix a silliness
of how we put the `position: absolute` stuff into the component-y part
of the hanger spinner instead of this specific use case lol oops!
Oh oops, this is the first script on the page with the `defer`
attribute, which means it needs to run before other scripts with
`defer`—and in this moment, it's not loading for me, which means the
pages aren't working!
I assume Plausible told me to use `defer` rather than `async` because
it expects the page to be ready; okay! Let's just move this to the
very body of the `<head>` instead, so it isn't taking priority over
anything else.
This doesn't actually really matter, because this doesn't actually get
used in the app right now? But I figure, hey it's not hard to maintain,
let's just do it for consistency!
The most notable thing here is that we keep the movie iframes running!
So if you're trying different pets for an animated item, the animation
keeps going while the new pet layers load alongside it.
This is also nice for like, the species/color picker form, so we're not
taking away input elements from people who depend on e.g. keyboard
focus.
Not using this on the item page preview yet, but we will!
I like this approach over e.g. a web component specifically for the
sandboxing: while I don't exactly *distrust* JS that we're loading from
Neopets.com, I don't like the idea of *any* part of the site that
executes arbitrary JS unsafely at runtime, even if we theoretically
trust where it theoretically came from. I don't want any failure
upstream to have effects on us!
I copied basically all of the JS from a related project
`impress-media-server` that I had spun up at one point, to investigate
similar embed techniques. Easy peasy drop-in-squeezy!
Instead of doing all this listening to Turbo events etc to know when
outfit layers might have changed, making it a custom element and wiring
in the behavior to its actual lifecycle makes it always Just Work!
This is a cute thing that I think sets us up for other stuff down the
line: move more of the outfit appearance logic into the `Outfit` class!
Now, we set up the item page with a temporary instance of `Outfit`,
then ask for its `visible_layers`.
Still missing restricted-zones logic and such, that's next!
Nice, gotta say, this is a pretty neat way of making things feel more
app-y! There's some missing pieces here about like, loading state etc,
but the vibes are pretty good, and the implementation was dead-easy!
Just stripping out the big React component, and having Rails output it!
There's a lot of work rn in extracting the Impress 2020 dependency from
the `wardrobe-2020` React app, and I'm just curious to see if we can
simplify it at all by pulling this stuff *way* back to basics, and
deleting the item page part of `wardrobe-2020` altogether.
In this draft, we regress a lot of functionality: it just shows the
item on a Blue Acara, with no ability to change it! I'm gonna play with
putting more of that back in.
I also haven't actually removed any of the item page React code; I just
stopped calling it. That can be a cleanup for another time, once we're
confident in this experiment!
I think the parens are silly now that this paragraph is just kinda all
bonus clarification info anyway. And I wanted to explain the cost
computation for the potions, and highlight the bundle thing!
If the item names are long, it helps to give them more room to breathe!
Whereas if they're short, it looks silly and makes it harder to scan
the table.
Just an extra bit of help for e.g. Dyeworks items with long names!
The table layout algo can get a bit funky about how it assigns extra
space, I want to encourage things like "Total: 5 items" etc not to
wrap, esp in the Dyeworks case where it's quite long!
There's more and more going on in here! Let's omit the base item name,
increase the table width a bit in this case, and tweak the rest a bit
while we're here.
I uhhh literally didn't know Dyeworks was a gacha system until Kaye
from the Owls team told me lmao
I should maybe uhh read more guides instead of assuming I've osmosed
things correctly oops!
Oh weird, even with `flush: true`, `content_for` will ignore an empty
block and *not* flush out the previous content. This could cause rows
whose subtitles *should* have been empty (e.g. no NC trade value) to
display the previous row's value instead.
Let's make this whole situation a bit more robust by having the
*template* clear out the subtitle right before calling the block. That
way, a previous row's value *can't* get in, no matter what.
I think it helps a bit to have only the label be dotted-underlined, to
hint that I'm offering help about what that *means*, but clear the way
for the value itself to be more visible and less cluttered.
I thought to myself, "I wonder if it's possible to use a sneaky hacky
`content_for` trick to be able to run this code in the template." And
indeed it is!
It's tricky cuz like, I want to render this template, and I want to
provide _multiple_ slots of content to it. So, in this variant, we keep
the block as being primarily for the actions, but also optionally
accept `content_for :subtitle` inside that block, too.
Executing that correctly is a bit tricky! The subtitle comes *before*
the actions. So, we `yield` the actions block immediately, save it to a
variable, and *then* get the subtitle block.
Previously, I added a Dyeworks section that was incorrect: the base
item being available in the NC Mall does *not* mean you can necessarily
dye it with a potion!
In this change, we lean on Owls to tell us more about Dyeworks status,
and only group items in this section that Owls has marked as "Permanent
Dyeworks".
We don't have support for limited-time Dyeworks items yet—I've sent out
a message asking the Owls team for more info on what they do for those
items!
I started writing this up, then sent a preview to a friend, and he was
like "oh cool, but also this is not correct?"
I didn't realize Dyeworks has limited-time support to be *able* to dye
certain items. Hey, glad we're writing this guide for people like me,
then! lol
I wonder if we can lean on Owls for this. It seems like they already
list "Permanent Dyeworks" for some items, I wonder if they say
something special for active limited-edition Dyeworks items!
We could do a whole thing about like, checking singular vs plural, but
I'd rather just keep it simpler; I think it's clear from context that
we're talking about a category, so plural is fine even if it's not
actually more than one.
Okay so, like 30 minutes ago I added fallback behavior for cases where
we can't correctly infer the color from a PB item's name… and then I
pulled it up in the color and found that, oh, right, there are already
3 PB items that *correctly* return `nil` for `Item#pb_color`: Aisha
Collar, Elephante Hat, and Ixi Collar.
This is because they're common items that apply to many colors, like
the basics, but also many other less-special or older color variants.
They are the most likely case where we'll return `nil`.
So, I've updated our fallback UI to, instead of talk vaguely about
missing data, just assume that we're dealing with basic items. In the
rare window of time where a new color is released, and we have PB items
for it but no manual color data yet, this can just incorrectly say
"Basic Colors" and that's fine.
Oh right, it's possible for `Item#pb?` to return true, but
`Item#pb_color` to return `nil`, if the item has the paintbrush item
description but we can't find a color whose name matches the item name.
This would be expected if a new color were added to Neopets, and PB
items for it were modeled by the community, but we hadn't manually
added the color to the database yet.
Previously, the Item Getting Guide would crash in this scenario. Now,
it correctly handles the possibility of a `nil` value for `pb_color`,
and shows some placeholder info.
To test this, I temporarily edited some item names to not contain the
color name anymore (e.g. "P-rate Elephante Shirt and Vest"), then
loaded the guide and made changes until it no longer crashed.
Oh oops, I added an optional `subtitle` argument for the item table
list row template, but didn't realize that it would crash in cases when
not provided! Now, we add an initializer to set it to `nil` if
undefined.
Note that there's a known performance issue here: we should try to
fetch all the OWLS values at once, instead of doing them in sequence
while rendering the page!
Now, for colors like Mutant or Magma where there's no paint brush
image to show, we use a sample pet image instead, to help it have equal
visual weight and clarity as the cases with the paint brushes.
We do some cleverness in here to make sure to always show the relevant
species, if possible!
Ohh yeah, this helps communicate the process much better, especially
for what the Shops/Trades links mean.
I think I'm gonna also go get the paint brush thumbnail images and add
them to the database too, to help better communicate that this is a
paint brush item situation.
Clearer focus states, row changes bg color on hover/focus to help you
track where you are, remove the redundant image thumbnail link from the
tab order (it's the same as the item name link!)
As part of this, I added a new `search_icon` helper, and a new
`button_link_to` helper, which both styles the link as a button and
accepts an `icon` parameter to make it easier to pass in an icon!
This helps us be more efficient with our use of space, keep the CTAs well
aligned, show a clear total, and set up how we might do CTAs for more complex
cases like all the potential Neopoint CTAs like Wiz/Trades/Auction/etc!
Now that we have this helper, we no longer need these stylesheets to
include a `body.controller-action` wrapper to scope all the styles!
Someday we should convert more of our stylesheets to this format,
instead of slamming them all into `application.sass` like we do now.
Ah, well!
NC prices, some CSS, and also a new application-level helper that adds
a feature I've long wanted and been working around for Turbo: the
ability to specific that a stylesheet is specific to the current page,
and should be unloaded when removed!
I use this to write `sources.sass` without the usual
`body.items-sources` scoping that we've historically used to control
what pages a stylesheet applies to. (In the long past, this was because
a lot of stylesheets were—and still are–routed through the
`application.sass` stylesheet! But even for more recent standalone page
stylesheets, I've done the scoping, to avoid issues with styles leaking
beyond the page they're meant for when Turbo does a navigation.)
This'll affect the recommended acquisition method by a lot!
NC Mall info like current price isn't surfaced anywhere else in the app
right now. It'd probably be good to add to the item page, and maybe
some other places too!
TNT requested that we figure out ways to connect the dots between
people's intentions on DTI to their purchases in the NC Mall.
But rather than just slam ad links everywhere, our plan is to design an
actually useful feature about it: the "Item Getting Guide". It'll break
down items by how you can actually get them (NP economy, NC Mall,
retired NC, Dyeworks, etc), and we're planning some cute actions you
can take, like shortcuts for getting them onto trade wishlists or into
your NC Mall cart.
This is just a little demo version of the page, just breaking down
items specified in the URL into NC/NP/PB! Later we'll do more granular
breakdown than this, with more info and actions—and we'll also like,
link to it at all, which isn't the case yet! (The main way we expect
people to get here is by a "Get these items" button we'll add to the
outfit editor, but there might be other paths, too.)
I refresh the image and UI color here to draw attention to the change!
I also delete the `neopass-thumbnail.png` image, since it's no longer
used anywhere anymore, but I would not be surprised if we want it back
someday and need to revive it from history!
Just a lil blurb to make sure it's clear that NC sales and stuff are
forbidden! I imagine the people doing it know this, but I want to make
sure we're being explicit, in case there's any element of
miscommunication.
In particular, we got feedback that it was surprising to not get to
check which NeoPass you wanted to use, and that the permissions were
never prompted again. I figure let's err on the side of ample clarity!
As part of this, I've added the new `external_link_icon` global helper,
which embeds an SVG from Chakra UI. That's just the convenient place I
know to grab that icon, and I did it this way instead of an `img` tag
because that enables the `currentColor` thing to work instead of coming
out black!
Not getting a lot of takers, I think it was wise to start small just in
case, but there doesn't seem to be a floodgate problem, so let's remove
the limitations and increase the ask! (But still not a full launch yet,
because I want to funnel people through the feedback process first.)
Got the icon and background style from Neopets.com! I didn't quite copy
the whole button style, both because getting it to play nice with our
existing styles didn't *immediately* work, but also because I think
this works out as a really good compromise between our two styles
anyway!
Ahh okay tricky lil thing: if you show the settings page with a partial
change to `AuthUser` that didn't get saved, it can throw off the state
of some stuff. For example, if you don't have a password yet, then
enter a new password but leave the confirmation box blank, then you'll
correctly see "Password confirmation can't be blank", but you'll *also*
then be prompted for your "Current password", even though you don't
have one yet, because `@auth_user.uses_password?` is true now.
In this change, we extend the Settings form to use two copies of the
`AuthUser`. One is the copy with changes on it, and the other is the
"persisted" copy, which we check for parts of the UI that care about
what's actually saved, vs form state.
Simplified this a bit into a helper. It's kinda odd to me, but
convenient for this moment, that Rails allows views to read `params`! I
guess it's for escape hatches exactly like this! lol
including validation logic to make sure it's not already connected to
another one!
The `intent` param on the NeoPass form is part of the key! Thanks
OmniAuth for making it easy to pass that data through!
I'm getting ready to add handling for "what if you don't *have* a
current password*??", so it seems like the right way to do that is to
just eject the controller and start customizing!
This is more consistent with the `uses_omniauth?` we already have, and
it also will help for the next change, where I want a `uses_password?`
method (and using the name `password?` breaks some of Devise's
validation code).
Ahh right, in development `User` and `AuthUser` will have the same ID,
but that got messed up early on for us in production DTI 😅
Here, we switch the form to reference the `User` instead of the
`AuthUser` (to get the ID right), then we also change how we compare
the IDs, because `User#to_param` appends extra text onto the ID after
the number!
