Add dropdown filter to show rooms, languages, session types. Ensure it works in mobile view #228
Conversation
|
|
Reviewer's Guide by SourceryThis pull request implements a new filtering system for the schedule and sessions pages. It replaces the previous single filter button with dropdown filters for tracks, rooms, and session types. The changes also include improvements to the mobile view and the addition of a "favorites only" filter. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider optimizing the
filteredTrackscomputed property for performance, especially if it's called frequently. - The addition of lodash for a single function (
_.uniqBy) might be unnecessary. Consider using native JavaScript methods instead to reduce dependencies.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| </template> | ||
| <script> | ||
| import _ from 'lodash' |
There was a problem hiding this comment.
suggestion (performance): Consider removing lodash dependency
While lodash is powerful, it might be overkill for this use case. Consider using native JavaScript methods to achieve the same functionality, which could reduce bundle size and improve performance.
// Remove this line if lodash is not used elsewhere in the file
// import _ from 'lodash'
// If specific lodash functions are needed, import them individually:
// import { functionName } from 'lodash-es'
| ...mapGetters('schedule', ['days', 'rooms', 'sessions', 'favs']), | ||
| exportType () { | ||
| return exportTypeSet | ||
| } | ||
| }, |
There was a problem hiding this comment.
suggestion (performance): Optimize filteredTracks computed property
This computed property seems to be doing a lot of work on each computation. Consider memoizing the results or using a more efficient filtering algorithm to improve performance, especially for large datasets.
filteredTracks() {
return Object.keys(this.filter).reduce((results, key) => {
const { refKey, data } = this.filter[key];
const selectedIds = data.filter(t => t.selected).map(t => t.value);
if (selectedIds.length) {
const filteredTalks = this.schedule.talks.filter(t => selectedIds.includes(t[refKey]));
return results ? filteredTalks.filter(t => results.includes(t.id)) : filteredTalks;
}
return results;
}, null)?.map(t => t.id) || null;
},
| ...mapGetters('schedule', ['days', 'rooms', 'sessions', 'favs']), | ||
| exportType () { | ||
| return exportTypeSet | ||
| } | ||
| }, |
There was a problem hiding this comment.
suggestion: Move complex logic to Vuex store
The sessions computed property contains complex logic that might be better suited in the Vuex store. This would improve separation of concerns and potentially allow for better caching and reusability across components.
sessions() {
return this.$store.getters['schedule/filteredSessions']({
onlyFavs: this.onlyFavs,
favs: this.favs,
filteredTracks: this.filteredTracks,
currentTimezone: this.currentTimezone
})
},
| state.schedule.session_type = state.schedule.talks.reduce((acc, current) => { | ||
| const isDuplicate = acc.some(item => item.session_type === current.session_type); | ||
| if (!isDuplicate) { | ||
| acc.push(current); | ||
| } | ||
| return acc; | ||
| }, []); |
There was a problem hiding this comment.
suggestion: Move session type processing to a separate action
The processing of session types could be moved to a separate action or mutation for better organization and potential reusability. This would also make the main action cleaner and easier to understand.
// In store/schedule.js
const processSessionTypes = (talks) => {
return talks.reduce((acc, current) => {
if (!acc.some(item => item.session_type === current.session_type)) {
acc.push(current);
}
return acc;
}, []);
};
// In the action
state.schedule.session_type = processSessionTypes(state.schedule.talks);
| @@ -47,6 +47,10 @@ | |||
| if (!state.schedule) return {} | |||
| return state.schedule.speakers.reduce((acc, s) => { acc[s.code] = s; return acc }, {}) | |||
| }, | |||
| sessionTypeLookup (state) { | |||
There was a problem hiding this comment.
issue (complexity): Consider consolidating logic to avoid redundancy.
The new code introduces some complexity that could be streamlined. The sessionTypeLookup function adds redundancy similar to tracksLookup and speakersLookup, which can complicate maintenance. Consider consolidating this logic to avoid duplication. The fetch action now includes data processing, which should ideally be separated from data fetching to maintain clear separation of concerns. Direct state mutation within actions can lead to side effects; using mutations to commit processed data to the state is more Vuex-compliant. Additionally, inconsistent naming, such as session_type used in different contexts, can be confusing. Consistent naming conventions would improve readability. Refactoring these aspects could enhance maintainability and clarity.
This PR closes/references issue #194 . It does so by:
How has this been tested?
Checklist
Summary by Sourcery
Add dropdown filters for tracks, rooms, and session types on the schedule and session pages, and ensure they are responsive for mobile view.
New Features:
Enhancements: