Add online video link to menu #150
Conversation
Reviewer's Guide by SourceryThis pull request adds a new 'video_link' field to the event settings form, allowing organizers to configure an online video link. This link will be displayed as a button in the event menu if set. File-Level Changes
Tips
|
|
Video url in event setting
Video button on talk menu
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @lcduong - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding more context to the PR description about the feature and its intended use.
- While not necessary for this PR, consider adding unit tests for this feature in the future to ensure its functionality is maintained.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟡 Security: 1 issue found
- 🟢 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.
| @@ -18,6 +18,11 @@ | |||
| <i class="fa fa-group"></i> {% translate "Tickets" %} | |||
| </a> | |||
| {% endif %} | |||
| {% if request.event.display_settings.video_link %} | |||
| <a href="{{ request.event.display_settings.video_link }}" target="_blank" class="btn btn-outline-success"> | |||
There was a problem hiding this comment.
🚨 suggestion (security): Enhance security for external link and improve accessibility
Consider adding rel="noopener noreferrer" to the link for security when opening in a new tab. Also, you might want to use a more specific label like "Live Stream" or "Video Stream" for better clarity.
| <a href="{{ request.event.display_settings.video_link }}" target="_blank" class="btn btn-outline-success"> | |
| <a href="{{ request.event.display_settings.video_link }}" target="_blank" rel="noopener noreferrer" class="btn btn-outline-success"> | |
| <i class="fa fa-video-camera"></i> {% translate "Video Stream" %} | |
| </a> |
| @@ -18,6 +18,11 @@ | |||
| <i class="fa fa-group"></i> {% translate "Tickets" %} | |||
| </a> | |||
| {% endif %} | |||
| {% if request.event.display_settings.video_link %} | |||
| <a href="{{ request.event.display_settings.video_link }}" target="_blank" class="btn btn-outline-success"> | |||
| <i class="fa fa-video-camera"></i> {% translate "Videos" %} | |||
There was a problem hiding this comment.
suggestion: Consider using a more modern Font Awesome icon
If you're using a recent version of Font Awesome, you might want to use 'fa-video' instead of 'fa-video-camera' for a more modern look.
| <i class="fa fa-video-camera"></i> {% translate "Videos" %} | |
| <i class="fa fa-video"></i> {% translate "Videos" %} |
| @@ -120,6 +120,16 @@ class EventForm(ReadOnlyFlag, I18nHelpText, JsonSubfieldMixin, I18nModelForm): | |||
| }), | |||
| required=False, | |||
| ) | |||
| video_link = forms.URLField( | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the URL field definitions to reduce redundancy and improve maintainability.
The new code introduces some complexity and redundancy by adding a video_link field that is very similar to the existing ticket_link field. This can lead to maintenance issues and makes the form definition longer and harder to read. To improve maintainability and readability, consider refactoring the code to use a common method for defining URL fields. Here's an example:
def create_url_field(label, help_text, placeholder):
return forms.URLField(
label=label,
help_text=help_text,
widget=forms.TextInput(attrs={'placeholder': placeholder}),
required=False,
)
class EventForm(forms.Form):
ticket_link = create_url_field(
label=_("Event ticket shop URL"),
help_text=_("Ticket shop link will be shown on event menu."),
placeholder='e.g: https://tickets-dev.eventyay.com/2024/wikimania/'
)
video_link = create_url_field(
label=_("Video Live URL"),
help_text=_("Online video link will be shown on event menu."),
placeholder='e.g: https://wikimania-live.eventyay.com/'
)
header_pattern = forms.ChoiceField(
label=_("Frontpage header pattern"),
help_text=_(
"Choose how the frontpage header banner will be styled if you don’t upload an image. Pattern source: "
'<a href="http://www.heropatterns.com/">heropatterns.com</a>, CC BY 4.0.'
),
choices=(
# Add your choices here
),
required=False,
)
json_fields = {
"imprint_url": "display_settings",
"show_schedule": "feature_flags",
"schedule": "display_settings",
"show_featured": "feature_flags",
"use_feedback": "feature_flags",
"export_html_on_release": "feature_flags",
"html_export_url": "display_settings",
"ticket_link": "display_settings",
"video_link": "display_settings",
"header_pattern": "display_settings",
"meta_noindex": "display_settings",
}This refactoring reduces redundancy and makes the code easier to maintain.
|
popup if customer not order ticket or not login yet. configuration need for this feature to working
|
This PR closes/references issue #133. It does so by
How has this been tested?
Checklist
Summary by Sourcery
This pull request introduces a new feature allowing event organizers to add an online video link in the event settings. The video link, if configured, will be displayed in the event menu for easy access.