Skip to content

Conversation

@marwan-at-work
Copy link
Contributor

@marwan-at-work marwan-at-work commented Jul 6, 2023

This PR is a follow up on #94 and adds a snooze option with a message to let users know about the available on/off settings.

Updates #80

Screenshot 2023-07-06 at 10 24 05 AM Screenshot 2023-07-06 at 10 24 21 AM

Feel free to suggest a different copy/UX.

Base automatically changed from marwan/pdcfg to main July 10, 2023 22:31

async snooze() {
this.snoozing = true;
setTimeout(() => {
Copy link

@tylersmalley tylersmalley Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only going to snooze for 15 minutes within the instance of VS Code it was triggered in. I would like a prompt for the duration, including day, week, and month. For this, I am thinking we could use context.globalStoragePath and persist the port config (snooze info) to disk.

Thoughts @samlinville @christine-tailscale

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When does this notification show up? Will the snooze function delay just this message or will it snooze all notifications from us?

I'm assuming that users would want to snooze because they're in the middle of something else and want to save it for later. What if instead of offering them custom snooze options, we offer them a quick yes/no decision? And then if they click no, we can use that notification to educate them where port discovery lives if they want to come back to it later?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The notification shows up as part of port discovery and as this PR stands snoozes discovery entirely:

image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should persist in the requested snooze duration (storing the time when we can unsnooze). We should not offer a calendar or complex form to fill out (we're trying to work around an annoyance, let's not annoy them more). Let's keep it simple.

15 minutes seems too short, TBH. The goal is to provide a reasonable snooze option that keeps the port disco from annoying users. If snooze is too short, we're still annoying them. The minimum risk is that the person will disable port disco notifications altogether. The max is the uninstall.

I wouldn't oppose a single "Snooze for 1hr" or "Snooze" and then prompting with 1 hour, 2 hours, or this session. (For example)

@christine-tailscale
Copy link

christine-tailscale commented Jul 11, 2023

Is this the right place for copy feedback? Wondering if we can update the toast notification to use active voice.

Update
Port 4545 was started by "printserver", would you like to share it over the internet with Tailscale Funnel? [Serve] [Snooze]
to
"Printserver" started port 4545. Would you like to expose this port to the internet using Tailscale Funnel? [Expose] [Not now] [Learn more] <-- Learn more could link to Funnel docs

@tylersmalley
Copy link

@christine-tailscale of course, all places are accepting of copy feedback!

@tylersmalley tylersmalley force-pushed the marwan/snooze branch 3 times, most recently from 163a7a8 to 5a4e837 Compare July 11, 2023 17:04
this.snoozing = true;
setTimeout(() => {
this.snoozing = false;
}, 900000); // fifteen minutes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer const snoozeDuration = 15 * 60 * 1000 then reference the const.

Signed-off-by: Naman Sood <mail@nsood.in>
Signed-off-by: Naman Sood <mail@nsood.in>
Signed-off-by: Naman Sood <mail@nsood.in>
@tendstofortytwo tendstofortytwo self-assigned this Oct 6, 2023
Signed-off-by: Naman Sood <mail@nsood.in>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants