-
Notifications
You must be signed in to change notification settings - Fork 8.9k
Add display name and icon to SUI extensions page #18633
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
Add display name and icon to SUI extensions page #18633
Conversation
src/cascadia/TerminalSettingsModel/PowershellCoreProfileGenerator.cpp
Outdated
Show resolved
Hide resolved
| <!-- | ||
| BODGY | ||
| Theoretically, you could use a ContentTemplateSelector directly. However, that doesn't work. | ||
| For some reason, we just get the object type's ToString called and the selector gets nullptr as a parameter. | ||
| Adding the template as a view model property is a workaround. | ||
| --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 FYI, I spent like most of today figuring out how to work around this. Frustrating 😡
| _extensionsVM = *extensionsVMImpl; | ||
| _extensionsViewModelChangedRevoker = _extensionsVM.PropertyChanged(winrt::auto_revoke, [=](auto&&, const PropertyChangedEventArgs& args) { | ||
| const auto settingName{ args.PropertyName() }; | ||
| if (settingName == L"CurrentExtensionSource") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 All the changes in this file are really just because I switched from CurrentExtensionSource to CurrentExtensionPackage. Honestly, it's better this way imo. It aligns better with how the New Tab Menu works as well as the other subpages for profiles
| WINRT_OBSERVABLE_PROPERTY(Editor::NewTabMenuViewModel, ViewModel, _PropertyChangedHandlers, nullptr); | ||
|
|
||
| private: | ||
| Editor::NewTabMenuEntryTemplateSelector _entryTemplateSelector{ nullptr }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝Drive by. We weren't using it at all.
This comment was marked as resolved.
This comment was marked as resolved.
ec88b32 to
7cdbb7c
Compare
f780523 to
cb04e7f
Compare
lhecker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a few nits, but they were so few and far between that I felt like it's not worth bringing up at all. I think the blurry icons are the only major issue.
This comment was marked as outdated.
This comment was marked as outdated.
src/cascadia/TerminalSettingsModel/AzureCloudShellGenerator.cpp
Outdated
Show resolved
Hide resolved
f650f45 to
b963d60
Compare
b963d60 to
f25e6fe
Compare
f25e6fe
into
dev/cazamor/sui/extensions-page



Summary of the Pull Request
Adds some polish to the Extensions page in the Settings UI. This includes:
References and Relevant Issues
Targets #18559
Detailed Description of the Pull Request / Additional comments
DisplayNameandIcontoIDynamicProfileGeneratorDisplayNameandIconfrom app extensions and use themExtensionPackageis a new WinRT class to package a bunch ofFragmentSettingstogetherExtensionPackages rather than dynamically adding/removingFragmentSettings contentsCurrentExtensionSourcewithCurrentExtensionPackageExtensionPackageTemplateSelectoris used to displayExtensionPackages with metadata vs simple ones that just have a sourceValidation
✅ Keyboard navigation feels right
✅ Screen reader reads all info on screen properly
✅ Accessibility Insights FastPass found no issues
✅ "Discard changes" retains subpage, but removes any changes