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

Rename Basic to Visual Basic in Visual Studio options page #53134

Merged
merged 2 commits into from
Feb 16, 2022

Conversation

Youssef1313
Copy link
Member

This does the first point of the design meeting notes in #34990 (comment).

@Youssef1313 Youssef1313 requested a review from a team as a code owner May 4, 2021 07:13
@Youssef1313 Youssef1313 changed the title Basic -> Visual Basic Rename Basic to Visual Basic in Visual Studio options page May 4, 2021
@Youssef1313 Youssef1313 closed this May 4, 2021
@Youssef1313 Youssef1313 reopened this May 4, 2021
@Youssef1313 Youssef1313 marked this pull request as ready for review May 14, 2021 07:54
@Youssef1313
Copy link
Member Author

@jasonmalinowski Can we get this merged? Thanks!

@jinujoseph jinujoseph added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 7, 2022
@dotnet dotnet deleted a comment from azure-pipelines bot Jan 7, 2022
@jasonmalinowski
Copy link
Member

@Youssef1313 Thanks for the ping; restarted our tests here since the previous runs were most stale.

@jasonmalinowski
Copy link
Member

Also @Youssef1313 this will be probably a few days to merge since this may have quite the impact on things like documentation. For example, our docs team internally pointed out that pressing F1 when you're on our VB pages takes you to https://docs.microsoft.com/en-us/visualstudio/ide/reference/options-text-editor-basic-visual-basic?view=vs-2022 so we need to make sure that doesn't break either.

@Youssef1313
Copy link
Member Author

@jasonmalinowski This should be easy enough to fix if it broke. Would you be able to check if it's broke? I won't be able to run RoslynDeployment at the moment unfortunately. If it broke, we should be able to capture the new f1_keyword and add it to the article metadata.

@Youssef1313
Copy link
Member Author

@jasonmalinowski I tested and it seems that F1 isn't broken. It's still using "Basic"
I don't know why it works, but.. it works 😄

Comment on lines 35 to 37
<ProvideLanguageEditorOptionPage(GetType(AdvancedOptionPage), "Basic", Nothing, "Advanced", "#102", 10160)>
<ProvideLanguageEditorToolsOptionCategory("Basic", "Code Style", "#109")>
<ProvideLanguageEditorOptionPage(GetType(CodeStylePage), "Basic", "Code Style", "General", "#111", 10161)>
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably this is why F1 is still working.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I guess the platform is just producing a string based on the underlying key names. I tried to do a bit of digging to figure out where that lived but didn't find it quickly enough.

@jasonmalinowski
Copy link
Member

@Youssef1313 Hmm, I guess maybe some extra debugging in in order, but it may use the underlying language name since the display name would be subject to localization. And thanks for testing, I guess I'll remove that for my list for today. 😄

As far as this PR I have to chase a bit more internally to make sure we're ready for the more general documentation impact on this, since even if F1 works we will have other pages that at least need to have content updated.

@Youssef1313
Copy link
Member Author

@jasonmalinowski Any updates?

@jasonmalinowski
Copy link
Member

@Youssef1313 Thanks for the reminder -- just poked the internal teams and I think we're mostly good, just waiting for a confirmation.

@jasonmalinowski
Copy link
Member

OK, internal teams are good; just going to give this one more CI run since it's a bit stale.

@jasonmalinowski
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@jasonmalinowski jasonmalinowski merged commit 95812f4 into dotnet:main Feb 16, 2022
@ghost ghost added this to the Next milestone Feb 16, 2022
@jasonmalinowski
Copy link
Member

Merged, thanks for the change @Youssef1313!

@Youssef1313 Youssef1313 deleted the patch-31 branch February 16, 2022 19:45
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants