Conversation
This pulls in the changes on the `crates-io-fork` branch of https://github.com/LawnGnome/ember-cli-notifications, which gets us: * The fix in mansona/ember-cli-notifications#337 for `onClick` handlers. * A new `onDismiss` option.
LawnGnome
added
C-enhancement ✨
| * @returns {void} | ||
| */ | ||
| #writeDismissedSet(set) { | ||
| this.cookies.write(BANNER_MESSAGE_COOKIE_NAME, [...set.values()].join(',')); |
There was a problem hiding this comment.
cookies are usually restricted to ~4KB. I guess this most likely won't cause any issues for us, but I'm curious if there was a reason for using cookies over local storage (which I think has a ~5MB limit)
There was a problem hiding this comment.
The reason is that I'm old. Local storage probably would be better.
| let dismissFired = false; | ||
| let notification = state.info('Message', { | ||
| onDismiss: local => { | ||
| expect(local).toBe(notification); | ||
| dismissFired = true; | ||
| }, | ||
| }); | ||
|
|
||
| state.removeNotification(notification); | ||
| expect(dismissFired).toBe(true); |
There was a problem hiding this comment.
I guess this works well enough, so don't feel the need to refactor this, but if you wanted to then https://vitest.dev/api/mock.html might be an alternative for this pattern :)
| "patchedDependencies": { | ||
| "ember-cli-notifications@9.1.0": "patches/ember-cli-notifications@9.1.0.patch" | ||
| } |
There was a problem hiding this comment.
I'm not a huge fan... 😅
I was gonna propose opening an upstream PR, but apparently that had already happened about 4 years ago 😞
then I was gonna propose using resolutions to point to a fork, but admittedly the committed patch might ultimately be better since the fork could be deleted, preventing our project from building.
so I guess I'm okay with this as a temporary measure, since we'll hopefully be switching over to the Svelte app soon anyway :)
There was a problem hiding this comment.
so I guess I'm okay with this as a temporary measure, since we'll hopefully be switching over to the Svelte app soon anyway :)
Yeah; if I thought this was going to last more than (hopefully) a couple of months, I'd probably do something more robust like actually publish the forked package.
|
one thing I forgot during the first review... could you add tests? 😅 |
2 participants
Putting this up as a draft because I don't even know how I feel about it right now.
To recap, the problem:
onclickto the banner message works locally, but staging and prod apply a content security policy that disables it. This is good, if inconvenient.I also realised today that there was another missing feature: we want this to be dismissible and for the user's browser to persist that it has been dismissed, otherwise we're going to spend the next three and a bit weeks annoying our users in general, and popping a notification over the log in link for most users every time they open or reload a crates.io tab in particular.
I've gone through a few options here:
onclickto the content security policy as a one-off. I'm not super keen on this, because it's an implementation detail we're going to forget with how infrequently we useBANNER_MESSAGE, but it's possible.So, instead, this PR does a few things (this lines up with the commits; a commit-by-commit review might be the best way to review this):
onDismisshandler that we can use to implement the persistence support mentioned above.onDismissto the Svelte reimplementation of notifications.This may be overkill. We may not want the persistence behaviour in general moving forward, in which case the cookie handling could be simplified and we just revert it after May 1st. We may want to migrate banner messages to not use the notification support at all, but to appear as a top-of-page banner instead, in which case we have a different — but maybe simpler — set of problems. We may also just decide that we don't care very much about this docs.rs banner after all, and not do anything. (In that case, let's revert #13341 as well.)
But I want to hear what the rest of you all think.