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

Alternate implementation of common resource API #210

Closed
wants to merge 10 commits into from

Conversation

dlockhart
Copy link
Member

This is an alternate implementation of #208 that uses a LocalizeCommon localizer class instead of merging the common resources with the rest.

I feel like this is a bit cleaner, but would require the creation of a LocalizeCommonMixin in core that consuming components extend.

Thoughts?

@dlockhart dlockhart changed the title Alternate implementation Alternate implementation of common resource API Nov 14, 2024
@dbatiste
Copy link
Contributor

I feel like this is a bit cleaner, but would require the creation of a LocalizeCommonMixin in core that consuming components extend.

What would happen if their component required both their own terms as well as the common ones? Would the terms be in different sets, and if that's the case, would localize find the terms in the lower level mixin in their class?

@dlockhart
Copy link
Member Author

What would happen if their component required both their own terms as well as the common ones?

This is kind of already an issue today when multiple mixins need localization -- although we've definitely had some bugs crop up there as the order of the mixins matters. So yeah, it'd be nice to not compound that problem.

Another option would be to still just leverage LocalizeMixin only with a loadCommon: true configuration flag, and then internally it would just instantiate a new Localize() (like it's already doing) and new LocalizeCommon(). The consumer would differentiate by calling localize('term') vs. localizeCommon('term'). That would allow us to drop the intl-common: prefix from all the common term names -- which was required to avoid collisions with consumer terms.

Base automatically changed from GAUD-7171/localize-common-characters to main November 18, 2024 17:24
@dlockhart
Copy link
Member Author

Closing this for now, as we'll proceed with the approach #208. If later we decide to change our mind to something more along the lines of what's in this PR, it should be easy to switch LocalizeMixin over.

@dlockhart dlockhart closed this Nov 18, 2024
@dlockhart dlockhart deleted the GAUD-7171/localize-common-characters-alt branch November 18, 2024 17:24
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.

2 participants