-
Notifications
You must be signed in to change notification settings - Fork 357
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 CoreWidgetProvider & SSH Wallet widget #320
Conversation
using Microsoft.Windows.ApplicationModel.Resources; | ||
|
||
namespace CoreWidgetProvider.Helpers; | ||
public static class Resources |
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.
Doesn't DevHome already have a system for accessing resources? @AmelBawa-msft
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.
Ping @AmelBawa-msft. Is another Resources ok here?
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.
We do have class StringResource
, ideally we need a single implementation, unless the current one does not work for your use case. I do want to add an option for specifying CultureInfo format, IIRC by default we use CurrentCulture
, but we should allow more flexibility.
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 see. Would be nice to reuse that. However, it should probably be moved to DevHome.Common from DevHome/Services first so I'd prefer doing this in a follow-up PR
// What is faster, String.Replace, RegEx, or StringBuilder.Replace? It is String.Replace(). | ||
// https://learn.microsoft.com/en-us/archive/blogs/debuggingtoolbox/comparing-regex-replace-string-replace-and-stringbuilder-replace-which-has-better-performance | ||
var resourceString = GetResource(identifier, log); | ||
str = str.Replace($"%{identifier}%", resourceString); |
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.
string in C# are immutable. Looks like str is replaced for every identifier.
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.
yes.
Resources.cs taken from https://github.com/microsoft/devhomegithubextension/blob/main/src/GithubPlugin/Helpers/Resources.cs
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/SSHWalletWidget.cs
Outdated
Show resolved
Hide resolved
|
||
[JsonSourceGenerationOptions(WriteIndented = true)] | ||
[JsonSerializable(typeof(DataPayload))] | ||
internal partial class SourceGenerationContext : JsonSerializerContext |
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.
NIT: 1 class per file.
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProvider/Widgets/WidgetProvider.cs
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProviderPackage/CoreWidgetProviderPackage.wapproj
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProviderPackage/CoreWidgetProviderPackage.wapproj
Outdated
Show resolved
Hide resolved
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.
No icons? Do we have icons for the core widget?
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.
Not yet. Any ideas for the icon from MDL icons? If not, where to ask for help for icons?
CoreWidgetProvider/CoreWidgetProviderPackage/Package.appxmanifest
Outdated
Show resolved
Hide resolved
CoreWidgetProvider/CoreWidgetProviderPackage/Package.appxmanifest
Outdated
Show resolved
Hide resolved
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.
Please remove duplicate code.
Please address the side-effects.
Please remove ARM support.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
65ef9c3
to
7c651f6
Compare
7c651f6
to
2539b17
Compare
2539b17
to
599d970
Compare
600d533
to
607755d
Compare
607755d
to
d5c9f81
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Feedback was addressed, Asked cory and he said override was ok as Kristen is reviewing actively
Summary of the pull request
References and relevant issues
Detailed description of the pull request / Additional comments
Validation steps performed
PR checklist