-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Adding ResourceString Markup Extension #4319
base: main
Are you sure you want to change the base?
Adding ResourceString Markup Extension #4319
Conversation
Thanks HerrickSpencer for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
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.
Looks great! 😄
Just left a bunch of code style nits, but the functionality looks perfect!
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
…sion.cs Co-authored-by: Sergio Pedri <[email protected]>
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
…sion.cs Co-authored-by: Sergio Pedri <[email protected]>
…sion.cs Co-authored-by: Sergio Pedri <[email protected]>
…sion.cs Co-authored-by: Sergio Pedri <[email protected]>
…sion.cs Co-authored-by: Sergio Pedri <[email protected]>
@Sergio0694 I updated all per your comments. Thank you. |
Thanks! I've updated per all your comments.
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ResourceString/ResourceStringXaml.bind
Show resolved
Hide resolved
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
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.
Looks good so far. Mostly styling fixes.
There's also a large file called "how" I wasn't able to directly comment on. Appears to contain a lot of git history and may have been added by mistake, this will need to be removed.
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ResourceString/ResourceStringXaml.bind
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ResourceString/ResourceStringXaml.bind
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ResourceString/ResourceStringXaml.bind
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.SampleApp/SamplePages/ResourceString/ResourceStringXaml.bind
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
return GetValue(name); | ||
} | ||
|
||
// Not using ResourceContext.GetForCurrentView nor GetForViewIndependentUse |
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.
What was the reason for not using these APIs here? Can we add more clarification to the code comment?
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs
Outdated
Show resolved
Hide resolved
As pointed out in the original issue
Many applications use more than one .resw file and wouldn't be able to fully use this new Markup Extension. |
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 30 days of this comment. |
@HerrickSpencer wanted to check-in and see if you were planning to pick this back up again to finish? Let us know, thanks! |
Sure! I kind of let this drop off my radar since it wasn't touched in the last 2 months, but I can certainly take a stab at addressing the latest concerns and providing an optional solution. |
a342626
to
470e521
Compare
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
…esourceStringXaml.bind Microsoft.Toolkit.Uwp.UI/Extensions/Markup/ResourceStringExtension.cs Per comments in PR
…ional value in x:Bind compiled function
…hat accomodates no optional value in x:Bind compiled function
470e521
to
d22658b
Compare
FYI @rudyhuyn |
WinUI 3 doesn't support changing the language by x:uid without restarting the app microsoft/microsoft-ui-xaml#5940. I think when you reload the page, the language will be changed. Can |
Fixes
#4318
With the markup extension:
<Button Content="{str:ResourceString Name=ButtonText}"/>
As you see we are using a custom markup extension to bind your content strings. We are able to simply pass in the name of the resource we want to use and get the localized string back.
Now our resource file need only contain simple names without the properties, making it much cleaner and easier to read.
One feature I'm adding to this is the ability to specify a language as well if the developer wanted to get resources from another language context.
<Button Content="{str:ResourceString Name=ButtonText}", Language="es-ES"/>
PR Type
What kind of change does this PR introduce?
Feature
Sample app changes
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Other information