-
Notifications
You must be signed in to change notification settings - Fork 64
Support availableLanguages
key
#968
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
Support availableLanguages
key
#968
Conversation
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.
Hi Marcus!
I left some refactoring ideas and suggestions, everything else looks great, thank you!
'metadata.availableLocales': function availableLocalesWatcher(availableLocales) { | ||
AppStore.setAvailableLocales(availableLocales); | ||
AppStore.setAvailableLocales(this.metadata?.availableLanguages ?? availableLocales); | ||
}, | ||
'metadata.availableLanguages': function availableLanguagesWatcher(availableLanguages) { | ||
AppStore.setAvailableLocales(availableLanguages); |
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 could refactor this code by adding a computed property:
availableLanguages: ({ metadata: { availableLanguages, availableLocales } = {} }) => (
availableLanguages ?? availableLocales
),
and then just watch that computed property like:
watch: {
availableLanguages(newValue) {
AppStore.setAvailableLocales(newValue);
},
},
instead of watching 'metadata.availableLocales'
and 'metadata.availableLanguages'
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 agree that the existing logic could be refactored and improved to be better and more consistent, but I was mostly focused on making the minimal changes to minimize the surface area of changes overall.
Thanks for the suggestion! It might be good to make those changes afterwards.
'metadata.availableLocales': function availableLocalesWatcher(availableLocales) { | ||
AppStore.setAvailableLocales(availableLocales); | ||
AppStore.setAvailableLocales(this.metadata?.availableLanguages ?? availableLocales); | ||
}, | ||
'metadata.availableLanguages': function availableLanguagesWatcher(availableLanguages) { | ||
AppStore.setAvailableLocales(availableLanguages); |
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.
Same feedback about the computed property as before
'metadata.availableLocales': function availableLocalesWatcher(availableLocales) { | ||
AppStore.setAvailableLocales(availableLocales); | ||
AppStore.setAvailableLocales(this.metadata?.availableLanguages ?? availableLocales); | ||
}, | ||
'metadata.availableLanguages': function availableLanguagesWatcher(availableLanguages) { | ||
AppStore.setAvailableLocales(availableLanguages); |
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.
Same feedback about the computed property as before
availableLanguages(availableLanguages) { | ||
AppStore.setAvailableLocales(availableLanguages); | ||
}, | ||
availableLocales(availableLocales) { | ||
AppStore.setAvailableLocales(availableLocales); | ||
AppStore.setAvailableLocales(this.availableLanguages ?? availableLocales); |
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.
Similar feedback:
We could make a computed property like
resolvedAvailableLanguages({ availableLanguages, availableLocales }) {
return availableLanguages ?? availableLocales;
}
and then have a watcher like
watch: {
resolvedAvailableLanguages(newValue) {
AppStore.setAvailableLocales(newValue);
},
},
on created()
, we could also simplify it by:
AppStore.setAvailableLocales(this.resolvedAvailableLanguages);
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.
Approving it since it works well!
@swift-ci test |
Bug/issue #, if applicable: 149303906
Summary
The
availableLocales
Render JSON key may be renamed toavailableLanguages
going forward, although we still want to support both keys for maximum compatibility with older JSON (preferring the new one if both are present for some reason).Testing
Steps:
availableLanguages
can be used in place ofavailableLocales
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
npm test
, and it succeeded