Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: send 1 notification when switching connection #420

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SethFalco
Copy link
Contributor

@SethFalco SethFalco commented May 28, 2022

Description

Previously, when one had 2 connections, switching created 2 notifications.

This isn't a major problem, but is a little annoying.

This instead adds a quiet argument to #disconnect, and uses this argument when switching to skip the notification for disconnecting.

Screencast.from.2024-06-07.23-52-02.webm

Checklist

  • New tests and/or benchmarks are included
  • Documentation is changed or added

Not sure if this PR requires them as this only really updates the use of showInformationMessage which is mocked in the tests anyway.

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Happy to discuss UX if you have any feedback/opinions.

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@SethFalco SethFalco changed the title fix: send 1 notification switch connection fix: send 1 notification when switching connection May 28, 2022
Copy link
Member

@Anemy Anemy left a comment

Choose a reason for hiding this comment

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

lgtm! One suggestion on a possible improvement.

src/connectionController.ts Outdated Show resolved Hide resolved
@SethFalco
Copy link
Contributor Author

I noticed you merged one of my PRs!
Just wanted to apologize for putting off these 2 that are still pending some work before they're ready to be reviewed. I'll try to get back to them tomorrow if I have time!

@alenakhineika
Copy link
Contributor

Hey @SethFalco, do you feel like picking up these PRs? They have been open for quite a while :) Or can we safely close them at this point?

@SethFalco
Copy link
Contributor Author

@alenakhineika Ahh, sorry about that! I actually forgot about this. A bit too much on my plate. 😓

If you don't mind, I would still like to address these PRs as I still use the extension. I can rebase and address it over the weekend.

And since last time I left maintainers hanging… if I don't update the PR for whatever reason by next week, feel free to close it. When I have time, I'm happy to open a new PR and reference this one.

@SethFalco
Copy link
Contributor Author

SethFalco commented May 26, 2024

I've amended the commit to mention the connection name on connect, disconnect, and both (switching between two connections).

I've included a video of all 3 notifications in the PR description. Any other feedback is welcome!

: 'MongoDB server';

void vscode.window.showInformationMessage(
`Disconnected from ${prevConnectionName} and connected to ${nextConnectionName}.`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`Disconnected from ${prevConnectionName} and connected to ${nextConnectionName}.`
`Changed the active connection to ${nextConnectionName}.`

We could simplify it and omit all logic responsible for prevConnectionName. Don't think the old connection details are important when connecting to a new cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rather than the word Changed, how about Set?

The word Changed implies the presence of a connection before, but if you'd like a single message for both an initial connection and changing connection, I think Set is more fitting.

@SethFalco SethFalco force-pushed the reduce-notification branch 2 times, most recently from 1712a84 to 6da3a87 Compare June 7, 2024 23:03
Co-authored-by: Alena Khineika <alena.khineika@gmail.com>
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.

3 participants