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

Adding Group by schema to OE #17543

Merged
merged 16 commits into from
Feb 21, 2023
Merged

Adding Group by schema to OE #17543

merged 16 commits into from
Feb 21, 2023

Conversation

aasimkhan30
Copy link
Contributor

@aasimkhan30 aasimkhan30 commented Feb 14, 2023

Menu item:
image
image

When feature enabled:
image
When feature disabled:
image

Setting will auto refresh:
GIF:
Code_zRwQATuUar

Light Mode:
image
Dark Mode:
image
HC:
image

Copy link
Contributor

@Charles-Gagnon Charles-Gagnon left a comment

Choose a reason for hiding this comment

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

This isn't very discoverable. Have you looking into adding inline action items and/or right click menu item support?

@aasimkhan30
Copy link
Contributor Author

@charles, added a menu item to connection OE tree

"command": "mssql.objectExplorer.enableGroupBySchema",
"when": "view == objectExplorer && !config.mssql.objectExplorer.groupBySchema",
"title": "%mssql.objectExplorer.enableGroupBySchema%",
"group": "secondary"
Copy link
Contributor

Choose a reason for hiding this comment

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

For VS Code MSSQL seems like it'd be better to have it as a navigation item so it shows up on the title bar. That's a lot easier to see and work with - the secondary menu is still a bit too hidden IMO.

@erinstellato-ms thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO I don't think people will toggle this setting a lot. Therefore, I am not sure about making it visible all the time. They will either stick with schema tree or regular tree. We can enable this setting by default in the next version and let users disable it if they want to.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not like there isn't plenty of room there. And I just think that hiding it in the secondary menu means that most people aren't going to know it exists unless they're really diligent about reading the release notes.

Just seems a shame to put all the work into this feature and then make it difficult for people to discover it. We can always hide the action later if people don't like it there.

@alanrenmsft
Copy link
Contributor

  1. any reason we are changing the icon for schema? previously it looks like this
    image
  2. I don't think we should be add the menu to the connection view as I mentioned earlier it is only applicable to MSSQL. Adding it to the connection context menu/inline action seems to be more appropriate, user might think it is a per connection setting, maybe we will see requests asking for per connection setting support.

@erinstellato-ms
Copy link

@Charles-Gagnon We (@aasimkhan30 and I) talked about how to make it more discoverable. I am planning to collect feedback from users once it is in the Insiders build to see what ideas/suggestions they have.

@alanrenmsft in terms of the icon, I honestly did like the one we had, it looked like a b-tree index to me. The new icon is (to me) more intuitive because it has the database + schema. Again, something I was planning to ask for user feedback on.

Happy to have a quick chat tomorrow if ya'll want to discuss more!

@dzsquared
Copy link
Contributor

As far as the ordering of the schemas - the default db_ schemas (everything before dbo) are the least likely item for users to engage with and probably should be artificially shuffled to the end.

@Charles-Gagnon
Copy link
Contributor

@alanrenmsft I'm suggesting the inline action button here because we only support MSSQL connections in the VS Code extension. I agree that finding some other way (such as context menu items) for ADS seems reasonable there.

package.json Outdated Show resolved Hide resolved
@aasimkhan30
Copy link
Contributor Author

aasimkhan30 commented Feb 15, 2023

  1. any reason we are changing the icon for schema? previously it looks like this
    image
  2. I don't think we should be add the menu to the connection view as I mentioned earlier it is only applicable to MSSQL. Adding it to the connection context menu/inline action seems to be more appropriate, user might think it is a per connection setting, maybe we will see requests asking for per connection setting support.

@alanrenmsft , it is fine to add the settings to connections view here because vscode-MSSQL extension only supports sql server connections

Copy link
Member

@kburtram kburtram left a comment

Choose a reason for hiding this comment

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

LGTM.

localization/xliff/enu/localizedPackage.json.enu.xlf Outdated Show resolved Hide resolved
Copy link
Contributor

@Charles-Gagnon Charles-Gagnon left a comment

Choose a reason for hiding this comment

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

I still think you should have this be a primary action on the title bar, but you and the PMs can make that decision.

@aasimkhan30 aasimkhan30 merged commit bcdec95 into main Feb 21, 2023
@aasimkhan30 aasimkhan30 deleted the aasim/feat/groupBySchema branch February 21, 2023 18:08
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