-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add tags to public schedule and session view #158
Conversation
Reviewer's Guide by SourceryThis pull request adds the functionality to display tags in the public schedule and session view. It includes changes to the JavaScript, Python views, models, templates, and SCSS files to support the display of tags with appropriate background and contrast colors. 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 @lcduong - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -53,6 +53,16 @@ def submission(self): | |||
def get_permission_object(self): | |||
return self.submission | |||
|
|||
def get_contrast_color(self, bg_color): |
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 moving contrast color calculation to a utility function
This method is duplicated in the frontend. Consider moving it to a shared utility function that can be used by both frontend and backend to maintain consistency and follow the DRY principle.
def get_contrast_color(self, bg_color): | |
from utils.color_utils import calculate_contrast_color | |
def get_contrast_color(self, bg_color): | |
if not bg_color: | |
return '' | |
return calculate_contrast_color(bg_color) |
@@ -337,6 +337,16 @@ def get_duration(self) -> int: | |||
return self.submission_type.default_duration | |||
return self.duration | |||
|
|||
def get_tag(self): |
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): Optimize tag retrieval to avoid potential N+1 query issue
This method might lead to multiple database queries when used in a loop. Consider using prefetch_related when querying submissions to load all tags in a single query, improving performance for schedules with many talks.
def get_tag(self): | |
def get_tag(self): | |
return list(self.tags.all().values_list('name', flat=True)) |
@@ -668,7 +668,8 @@ def build_data(self, all_talks=False, filter_updated=None, all_rooms=False): | |||
"updated": talk.updated.isoformat(), | |||
"state": talk.submission.state if all_talks else None, | |||
"fav_count": count_fav_talk(talk.submission.code) if talk.submission else 0, | |||
"do_not_record": talk.submission.do_not_record | |||
"do_not_record": talk.submission.do_not_record, | |||
"tags": talk.submission.get_tag() |
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.
issue (performance): Potential performance issue with tag retrieval in schedule building
Calling get_tag() for each talk might result in multiple database queries. Consider prefetching tags for all submissions at once before building the schedule data to improve performance.
@@ -93,6 +103,9 @@ | |||
.order_by("start") | |||
.select_related("room") | |||
) | |||
ctx["submission_tags"] = self.submission.tags.all() |
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.
issue (complexity): Consider refactoring the get_contrast_color
method and context preparation logic.
The new code introduces additional complexity for several reasons:
- Single Responsibility Principle: The
get_contrast_color
method seems unrelated to the primary responsibilities of the class, making the class harder to understand and maintain. - Inline Logic in Context Preparation: Adding logic directly into the context preparation section (
ctx["submission_tags"]
loop) makes it more complex and harder to read. - Code Duplication: Embedding the contrast color calculation logic directly in the context preparation could lead to code duplication if similar logic is needed elsewhere.
- Separation of Concerns: Mixing presentation logic (contrast color calculation) with data preparation makes the code harder to test and maintain.
Suggested Refactor
- Move the
get_contrast_color
method to a utility module: This keeps the class focused on its primary responsibilities. - Create a method to prepare submission tags: This encapsulates the logic for preparing the tags, making the context preparation cleaner.
# utils.py
def get_contrast_color(bg_color):
if not bg_color:
return ''
bg_color = bg_color.lstrip('#')
r = int(bg_color[0:2], 16)
g = int(bg_color[2:4], 16)
b = int(bg_color[4:6], 16)
brightness = (r * 299 + g * 587 + b * 114) / 1000
return 'black' if brightness > 128 else 'white'
# views.py
from .utils import get_contrast_color
class YourClass:
# ... existing methods ...
def prepare_submission_tags(self):
tags = self.submission.tags.all()
for tag_item in tags:
tag_item.contrast_color = get_contrast_color(tag_item.color)
return tags
def get_context_data(self, **kwargs):
ctx = super().get_context_data(**kwargs)
# ... existing context preparation logic ...
ctx["submission_tags"] = self.prepare_submission_tags()
return ctx
Benefits of the Refactor
- Separation of Concerns: The utility function
get_contrast_color
is moved to a separate module, making it reusable and easier to test. - Single Responsibility: The class methods are focused on their primary responsibilities.
- Readability: The context preparation logic is cleaner and easier to understand.
- Maintainability: The code is easier to maintain and extend in the future.
@@ -15747,6 +15749,25 @@ const markdownIt = markdown_it_default()({ | |||
} | |||
} | |||
|
|||
}, | |||
methods: { |
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.
issue (complexity): Consider refactoring the getContrastColor
method into a utility function and moving the tag rendering logic into a separate Tag
component.
The new code introduces increased complexity due to several factors:
- Increased Method Complexity: The
getContrastColor
method adds additional logic to theSession.vue
component, which could be abstracted away. - Inline Styles and Logic: The template now includes inline styles and logic for handling tags and their colors, making it more cluttered and harder to read.
- Additional Data Handling: Handling
tags
in thesessions
array introduces more data manipulation and rendering logic, increasing overall complexity. - CSS Changes: New CSS rules for handling tags and their styles add more complexity to the stylesheet.
To reduce complexity, consider refactoring the getContrastColor
method into a utility function and moving the tag rendering logic into a separate Tag
component. This will make the Session.vue
component cleaner and more maintainable. Here is a suggested refactor:
Utility Function (utils.js)
export function getContrastColor(bgColor) {
if (!bgColor) {
return '';
}
bgColor = bgColor.replace('#', '');
const r = parseInt(bgColor.slice(0, 2), 16);
const g = parseInt(bgColor.slice(2, 4), 16);
const b = parseInt(bgColor.slice(4, 6), 16);
const brightness = (r * 299 + g * 587 + b * 114) / 1000;
return brightness > 128 ? 'black' : 'white';
}
Tag Component (Tag.vue)
<template>
<div class="tag-item" :style="{ backgroundColor: tag.color, color: contrastColor }">
{{ tag.tag }}
</div>
</template>
<script>
import { getContrastColor } from '@/utils';
export default {
props: {
tag: {
type: Object,
required: true
}
},
computed: {
contrastColor() {
return getContrastColor(this.tag.color);
}
}
};
</script>
<style scoped>
.tag-item {
padding: 3px;
}
</style>
Updated Session.vue
<template>
<a :href="link" target="_vm.linkTarget" @click="onSessionLinkClick($event, session)">
<div class="time-box">
<div class="start" :class="{ 'has-ampm': hasAmPm }">
<div class="time">{{ startTime.time }}</div>
<div v-if="startTime.ampm" class="ampm">{{ startTime.ampm }}</div>
</div>
<div class="duration">{{ getPrettyDuration(session.start, session.end) }}</div>
<div class="buffer"></div>
<div v-if="isLive" class="is-live">live</div>
</div>
<div class="info">
<div class="title">{{ getLocalizedString(session.title) }}</div>
<div v-if="session.speakers" class="speakers">
<div class="avatars">
<img v-for="speaker in session.speakers" :src="speaker.avatar" v-if="speaker.avatar" />
</div>
<div class="names">{{ session.speakers.map(s => s.name).join(', ') }}</div>
</div>
<div v-if="session.do_not_record" class="do_not_record">
<svg viewBox="0 -1 24 24" width="22px" height="22px" fill="none" xmlns="http://www.w3.org/2000/svg">
<path d="M1.29292 20.2929C0.902398 20.6834 0.902398 21.3166 1.29292 21.7071C1.68345 22.0976 2.31661 22.0976 2.70714 21.7071L22.7071 1.70711C23.0977 1.31658 23.0977 0.68342 22.7071 0.29289C22.3166 -0.097631 21.6834 -0.097631 21.2929 0.29289L20.2975 1.28829C20.296 1.28982 20.2944 1.29135 20.2929 1.29289L2.29289 19.2929C2.29136 19.2944 2.28984 19.296 2.28832 19.2975L1.29292 20.2929z" fill="#758CA3"/>
<path d="M15 3C15.2339 3 15.4615 3.02676 15.68 3.07739L13.7574 5H3C2.44772 5 2 5.44771 2 6V16C2 16.2142 2.06734 16.4126 2.182 16.5754L0.87868 17.8787C0.839067 17.9183 0.800794 17.9587 0.76386 18C0.28884 17.4692 0 16.7683 0 16V6C0 4.34314 1.34315 3 3 3H15z" fill="#758CA3"/>
<path d="M10.2426 17H15C15.5523 17 16 16.5523 16 16V14.0233C15.9996 14.0079 15.9996 13.9924 16 13.9769V11.2426L18 9.2426V13.2792L22 14.6126V7.38742L18.7828 8.45982L21.9451 5.29754L22.6838 5.05132C23.3313 4.83547 24 5.31744 24 6V16C24 16.6826 23.3313 17.1645 22.6838 16.9487L18 15.3874V16C18 17.6569 16.6569 19 15 19H8.24264L10.2426 17z" fill="#758CA3"/>
</svg>
</div>
<div class="tags-box">
<Tag v-for="tag in session.tags" :key="tag.tag" :tag="tag" />
</div>
<div v-if="showAbstract" class="abstract" v-html="abstract"></div>
<div class="bottom-info">
<div v-if="session.track" class="track">{{ getLocalizedString(session.track.name) }}</div>
<div v-if="showRoom && session.room" class="room">{{ getLocalizedString(session.room.name) }}</div>
</div>
</div>
<bunt-icon-button class="btn-fav-container" @click.prevent.stop="faved ? $emit('unfav', session.id) : $emit('fav', session.id)">
<svg class="star" viewBox="0 0 24 24">
<path d="M12,17.27L18.18,21L16.54,13.97L22,9.24L14.81,8.62L12,2L9.19,8.62L2,9.24L7.45,13.97L5.82,21L12,17.27Z"/>
</svg>
</bunt-icon-button>
</a>
</template>
<script>
import Tag from './Tag.vue';
import markdownIt from 'markdown-it';
export default {
props: {
session: {
type: Object,
required: true
},
link: String,
linkTarget: String,
showAbstract: Boolean,
showRoom: Boolean,
faved: Boolean,
isLive: Boolean,
startTime: Object,
hasAmPm: Boolean
},
components: {
Tag
},
methods: {
onSessionLinkClick(event, session) {
// Your existing logic
},
getPrettyDuration(start, end) {
// Your existing logic
},
getLocalizedString(string) {
// Your existing logic
}
},
computed: {
abstract() {
try {
return markdownIt.renderInline(this.session.abstract);
} catch (error) {
return this.session.abstract;
}
}
}
};
</script>
<style scoped>
/* Your existing styles */
</style>
This refactor will help in reducing complexity and improving maintainability and readability of the codebase.
This PR closes/references issue #122 . It does so by
How has this been tested?
Checklist
Summary by Sourcery
This pull request introduces the display of tags in the public schedule and session views. It also includes a method to calculate contrast colors for tag backgrounds to improve text readability.