-
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 multi language filter track/session/room #237
Conversation
Reviewer's Guide by SourceryThis pull request implements multi-language support for filtering tracks, sessions, and rooms in the schedule view. The changes primarily affect the filtering logic and data preparation for the filter components, allowing for both string and object representations of session types, tracks, and room names to accommodate multiple languages. 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 - here's some feedback:
Overall Comments:
- Consider refactoring the language handling logic into a separate utility function or service to improve code organization and maintainability.
- The repeated code in both files suggests an opportunity for further abstraction. Consider creating shared functions for common operations.
- Instead of accessing localStorage directly in the component, consider creating a user preferences or configuration service to handle language settings.
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.
const setSessionType = new Set() | ||
const enLanguage = 'en' | ||
|
||
this.schedule.session_type.forEach(t => { |
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 extracting common language filtering logic into a separate function
The language filtering logic is repeated in multiple places. Creating a reusable function would improve maintainability and reduce code duplication.
function filterSessionTypes(schedule, language = 'en') {
const setSessionType = new Set()
schedule.session_type.forEach(t => {
if (typeof t.session_type === 'string' && t.language === language) {
setSessionType.add(t.session_type)
}
})
return setSessionType
}
const setSessionType = filterSessionTypes(this.schedule, enLanguage)
getLanguage() { | ||
return localStorage.getItem('userLanguage') || 'en' | ||
}, | ||
filterLanguage(data) { |
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: Extract duplicate code into a shared module or mixin
The changes in both 'index.vue' and 'sessions/index.vue' are nearly identical. Consider extracting this functionality into a shared module or mixin to improve maintainability and reduce duplication.
import { languageUtils } from '@/utils/languageUtils'
// ...
methods: {
filterLanguage(data) {
return languageUtils.filterLanguage(data, this.getLanguage())
},
…ideo into multi-language-filter
Summary by Sourcery
Introduce multi-language support for filtering tracks, sessions, and rooms in the schedule view by updating the filtering logic to handle session types with multiple language representations and adding utility functions to determine the user's language preference.
New Features:
Enhancements: