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

Dont update disabled extensions #58723

Merged
merged 6 commits into from
Sep 20, 2018
Merged

Dont update disabled extensions #58723

merged 6 commits into from
Sep 20, 2018

Conversation

ramya-rao-a
Copy link
Contributor

@ramya-rao-a ramya-rao-a commented Sep 14, 2018

Fixes #22461

This PR makes changes to avoid any update related UI for disabled extensions

cc @sandy081

@ramya-rao-a ramya-rao-a self-assigned this Sep 14, 2018
@sandy081
Copy link
Member

@ramya-rao-a It's an easy one but not sure making it default might make other users happy. Not showing the truth about an extension especially when others are showing may not be intended. We might have mitigated this issue already by not showing them in the default view. If this is necessary for users this can go behind a flag in my opinion. What do you think?

@ramya-rao-a
Copy link
Contributor Author

There are 3 UI locations related to extension updates

  • The badge in the activity bar
  • The Check for Extensions Updates action that results in a notification that tells you the number of updates
  • The Update to 1.2.3 button on the extension in the list.

In my opinion the count on the badge should definitely not include the disabled extensions. This is a pro-active information we show and are calling the user to take an action. And the action on disabled extensions is not urgent.

We could update the notification from Check for Extensions Updates action to include the count for enabled and disabled extensions separately. This is something that the user explicitly asked for, so including the disabled extensions count separately makes sense.

With the Check for Extension Updates now including the disabled extensions, it is a no brainer to include the Update to 1.2.3 action on the disabled extension.

I'll make the changes appropriately.

@ramya-rao-a
Copy link
Contributor Author

@sandy081 Done.

@sandy081
Copy link
Member

@ramya-rao-a I am still bit uncertain to show different update badge numbers depending on disabled extensions. Did not it confuse users if the two VS Code windows show two different badge numbers? If users really want that this feature I would go for opt in rather than giving default.

@ramya-rao-a
Copy link
Contributor Author

I am hesitant to add a new setting for a feature that will be used only by a very small section of our user base.

Updated the PR to update the hover text on the badge to show the same message as the Check for Extension Updates command. It will continue to show the total number of outdated extensions.

This should be a good middle ground for both sides of the argument.

@sandy081
Copy link
Member

@ramya-rao-a Sorry I think I buy your argument to have the badge showing only for enabled extensions. As you said it is prompting user to take some action and it should not when an extension is explicitly disabled. Please revert the last commit to go to previous change and push.

Thanks.

@sandy081 sandy081 added this to the September 2018 milestone Sep 20, 2018
@sandy081 sandy081 added the extensions Issues concerning extensions label Sep 20, 2018
@sandy081 sandy081 removed their request for review September 20, 2018 13:33
@sandy081
Copy link
Member

LGTM

@sandy081 sandy081 self-requested a review September 20, 2018 13:33
@ramya-rao-a
Copy link
Contributor Author

Reverted the last change.

@ramya-rao-a ramya-rao-a merged commit 96033fe into master Sep 20, 2018
@ramya-rao-a ramya-rao-a deleted the ramyar/outdated-disable branch September 20, 2018 17:39
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
extensions Issues concerning extensions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't update disabled extensions
2 participants