-
Notifications
You must be signed in to change notification settings - Fork 543
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
Localizable component #824
Conversation
[Parameter] | ||
public string ResourceKey { get; set; } | ||
|
||
protected string Localize(string name) |
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.
Just a little note here, I had seen T
is common name for translation, so can we use it instead?
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.
can you provide more detail on what you are asking?
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.
Just if we can rename Localize
to T
or just forget it
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.
My preference is to use Localize() as its much more explicit/intuitive - I would have no idea what T means.
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.
T for Translate, I'm with you, seems I need to add a check in every child component, cause we have 3 cases:
-
Localizable component and key is exist
-
Localizable component and key isn't exist
-
Non localizable component
I will do a final test before merge
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.
You can merge after the upcoming commit because there's a case needed where the resource key is not set |
@sbwalker it's ready to merge |
if (ModuleState?.ModuleType != null) | ||
{ | ||
var moduleType = Type.GetType(ModuleState.ModuleType); | ||
var localizerTypeName = $"Microsoft.Extensions.Localization.IStringLocalizer`1[[{moduleType.AssemblyQualifiedName}]], Microsoft.Extensions.Localization.Abstractions"; |
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.
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.
Oooh, how did I forgot that we already have IStringLocalizerFactory.Create()
;) I will try to simplify this soon
Thanks again
var localizerTypeName = $"Microsoft.Extensions.Localization.IStringLocalizer`1[[{moduleType.AssemblyQualifiedName}]], Microsoft.Extensions.Localization.Abstractions"; | ||
var localizerType = Type.GetType(localizerTypeName); | ||
|
||
// HACK: Use ServiceActivator instead of injecting IHttpContextAccessor, because HttpContext throws NRE in WebAssembly runtime |
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.
Using IHttpContextAccessor
in a Blazor Server app could expose you to security vulnerabilities. Components do not guarantee they execute in the context of a user's request (think a timer that's updated in the background or a chat component that causes other instances of chat components to update when you post a message). You should not use it in general since it's an anti-pattern, but definitely not in a server app.
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.
Thanks @pranavkm for pointing to this
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.
@hishamco are you going to be able to take a look at this? Or should I make an attempt at it?
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.
@sbwalker if you meant IStringLocalizerFactory
I already started today on this on, but one point I'm still think about, the factory will create the IStringLocalizer
but my aim in the PR was to retrieve it from the DI. I will update you on this soon, before releasing 2.0.0
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.
Seems something broken in localization, I will try to figure out what happen into the latest few PRs
It was a naming issue in my resources ;)
@sbwalker this tested and will make component localization more easier