-
Notifications
You must be signed in to change notification settings - Fork 23
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 mutiple language for filter track #217
Support mutiple language for filter track #217
Conversation
Reviewer's Guide by SourceryThis pull request adds support for multiple languages in the track filter of the schedule sessions view. The changes involve modifying the Vue component to dynamically display track names based on the user's language preference stored in localStorage. File-Level Changes
Tips
|
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.
Hey @odkhang - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
getTrackName(track) { | ||
const language_track = localStorage.userLanguage; | ||
if (typeof track.name === 'object' && track.name !== null) { | ||
if (language_track && track.name[language_track]) { | ||
return track.name[language_track]; | ||
} else { | ||
return track.name.en || track.name; | ||
} | ||
} else { | ||
return track.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.
suggestion: Consider refactoring getTrackName for better robustness and Vue.js integration
The current implementation directly accesses localStorage and may not handle all cases correctly. Consider using Vuex for managing user preferences and ensure the function always returns a string. Also, think about moving this logic to a computed property or a more central location for reusability.
computed: {
...mapState(['userLanguage']),
getTrackName() {
return (track) => {
if (typeof track.name === 'object' && track.name !== null) {
return track.name[this.userLanguage] || track.name.en || Object.values(track.name)[0] || '';
}
return String(track.name || '');
};
}
},
@@ -39,7 +39,7 @@ | |||
h1 {{ $t('Tracks')}} | |||
template(v-for="track in schedule.tracks") |
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.
suggestion (performance): Add a key to the v-for loop for better performance
When using v-for, it's a Vue.js best practice to provide a unique key for each item. This helps Vue optimize rendering and improves performance, especially for large lists. Consider adding :key="track.id" to your v-for loop.
template(v-for="track in schedule.tracks") | |
template(v-for="track in schedule.tracks" :key="track.id") |
Similar with #213
to support mutiple language for track filter
Summary by Sourcery
Support multiple languages for track filter by introducing a method to retrieve track names based on the user's language preference.
New Features: