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

"Launch" versus "open" and "go to" #32351

Closed
wants to merge 13 commits into from

Conversation

Jay-o-Way
Copy link
Collaborator

@Jay-o-Way Jay-o-Way commented Apr 9, 2024

Summary of the Pull Request

Lots of changed words, using

  • open in stead of launch (an app)
  • go to settings (for a specific page, mostly OOBE)
  • included a few strings in the tray icon menu

PR Checklist

  • Closes: "Open", "Launch", "Go to"... #30685
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Validation Steps Performed

Build and go through various windows.

image
image
image

Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way left a comment

Choose a reason for hiding this comment

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

Need some fixes

@@ -36,7 +36,7 @@ protected override void OnNavigatedFrom(NavigationEventArgs e)

private void Launch_EnvironmentVariables_Click(object sender, Microsoft.UI.Xaml.RoutedEventArgs e)
{
bool launchAdmin = SettingsRepository<EnvironmentVariablesSettings>.GetInstance(new SettingsUtils()).SettingsConfig.Properties.LaunchAdministrator;
bool launchAdmin = SettingsRepository<EnvironmentVariablesSettings>.GetInstance(new SettingsUtils()).SettingsConfig.Properties.OpenAsAdministrator;
Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way Apr 11, 2024

Choose a reason for hiding this comment

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

Wait, is this a breaking change ⁉️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be, because it will change the JSON stored name and also the DSC.

@crutkas
Copy link
Member

crutkas commented Apr 17, 2024

This is too big of a single change.

@Jay-o-Way
Copy link
Collaborator Author

Jay-o-Way commented Apr 17, 2024

This is too big of a single change.

@crutkas Why. And what do you propose?

@jaimecbernardo
Copy link
Collaborator

This is too big of a single change.

@crutkas Why. And what do you propose?

Hi @Jay-o-Way , here's some ideas for splitting it:

  • User facing strings. (which Clint is more interested in reviewing)
  • Code comments.
  • Actual variable names in the code (I'd definitely like this one to be splitted, since it impacts names on config files as well)

Another split that's helpful here is for the user facing strings, do:

  • Settings, since most of the changes are there.
  • Everything else.

@jaimecbernardo
Copy link
Collaborator

Thanks for opening the PR, by the way 😉

@Jay-o-Way
Copy link
Collaborator Author

@jaimecbernardo now THAT'S a comment I can work with! Thank's!

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.

None yet

3 participants