-
Notifications
You must be signed in to change notification settings - Fork 2
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
Allow for notices on all event pages #1185
base: main
Are you sure you want to change the base?
Conversation
@@ -1,5 +1,5 @@ | |||
<section class="row"> | |||
{% if has_agenda and not event.has_passed %} | |||
{% if has_agenda and event_notices %} |
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.
This branch is allowing for a notice to be posted even when an event has_passed (concluded). Because of this, the styling conditionals have been adjusted to allow space for notices if they exist at 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.
Also something to note, any notice showing up is still contingent on the event having an agenda, to mimic the way public comment instructions only displayed if they had one as well. Lmk if we want to change this!
{% for notice in event_notices %} | ||
{% if "upcoming" in notice.broadcast_conditions and event.is_upcoming %} | ||
{{ notice.message|safe }} | ||
{% elif "ongoing" in notice.broadcast_conditions and event.is_ongoing %} | ||
{{ notice.message|safe }} |
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.
This pattern makes it so that notices only ever show if the event's broadcast status matches the notifications' conditions. This is repeated across the public_comment
partials
@@ -1,5 +1,5 @@ | |||
<section class="row"> | |||
{% if has_agenda and not event.has_passed %} | |||
{% if has_agenda and event_notices %} |
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.
Also something to note, any notice showing up is still contingent on the event having an agenda, to mimic the way public comment instructions only displayed if they had one as well. Lmk if we want to change this!
class CheckboxSelectMultipleList(forms.CheckboxSelectMultiple): | ||
def format_value(self, value): | ||
if isinstance(value, str): | ||
return value.split(",") |
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.
This is helping to actually display our arrayfield choices in the create/edit views
{% if USING_ECOMMENT %} | ||
<p> | ||
<strong>On the web:</strong> | ||
<a href="{{ event.ecomment_url }}" target="_blank" aria-label="Go to public comment - link opens in a new tab"> | ||
<i class="fa fa-fw fa-external-link" aria-hidden="true"></i> | ||
Go to public comment | ||
</a> | ||
</p> | ||
{% endif %} |
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.
The ecomment portions of the partials have been kept for now.
# Only provide notices for this event's public comment setting | ||
if event.accepts_live_comment: | ||
context["event_notices"] = EventNotice.objects.filter( | ||
comment_conditions__contains=["accepts_live_comment"] | ||
) |
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.
Deciding which notices to pass through based off of public comment status in the view to take some burden off of the template.
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.
This is looking really snazzy! Left a few questions inline. How about also adding a link to edit the event notice to the Wagtail userbar? (See the hook that adds the manage alerts link.)
lametro/models/cms.py
Outdated
COMMENT_CONDITION_CHOICES = [ | ||
("accepts_live_comment", "Events that accept live public comments"), | ||
("accepts_comment", "Events that accept public comments when not live"), | ||
("accepts_no_comment", "Events that do not accept public comments at 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.
Hm, I'm a little confused by the comment conditions. I think all events accept comment?
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.
Ah okay, I was basing this off of the below section in the event template. Is this is a holdover from a time when not all events accepted comment, or maybe I'm just misinterpreting the below conditions?
la-metro-councilmatic/lametro/templates/event/event.html
Lines 109 to 116 in 431a943
<!-- Details --> | |
{% if not event.accepts_public_comment %} | |
{% include 'event/_event_header_no_public_comment.html' %} | |
{% elif event.accepts_live_comment %} | |
{% include 'event/_event_header_live_public_comment.html' %} | |
{% else %} | |
{% include 'event/_event_header_no_live_public_comment.html' %} | |
{% endif %} |
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.
Looks like some committee meetings don't accept comments: https://github.com/Metro-Records/la-metro-councilmatic/pull/823/files
But that's determined on the back end – this logic shouldn't be exposed in the CMS!
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.
Gotcha, I've removed it from the cms!
In that case, what do you think about me bringing back the public comment instructions in the template then? Since it says different things depending on which template is being rendered, and the same "upcoming" notice would render across all those templates regardless of whether the event accepted comment or not.
Otherwise we can keep it out and leave it up to them to decide on language that would make sense for all those public comment conditions.
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.
Hmmm.................................. maybe you were right to include the comment conditions after all. Can we change the labels to:
- Events that accept public comment before and during the meeting
- Events that accept public comment before but not during the meeting
- Events that do not accept public comment
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.
Sorry for the run around!
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.
No worries! It's an easy adjustment and I could've explained myself more haha.
@@ -53,78 +53,41 @@ | |||
{% endif %} | |||
</section> | |||
|
|||
{% if has_agenda and not event.has_passed %} | |||
{% if has_agenda and event_notices %} |
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.
I'm wondering now if it would be easier to get the appropriate notice/s in the view so we can simplify the templates?
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.
I think it would be easier once we figure out how to get those template variables to work within these notices. As of now, the conditionals that are left within these three comment templates have event specific variables in them that we wouldn't currently be able to render in the editable notice. Unless you had something else in mind!
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.
Got it, ok. Let's put a pin in this.
@@ -109,7 +109,7 @@ | |||
<ul class="nav col-md-6 justify-content-around justify-content-md-end list-unstyled"> | |||
<li class="ms-3"><a href="{% url 'lametro:contact' %}">Contact Us</a></li> | |||
<li class="ms-3"><a href="{% slugurl 'about' %}">About Us</a></li> | |||
<li class="ms-3"><a href="{% url 'metro_login' %}">Metro Login</a></li> | |||
<li class="ms-3"><a href="{% url 'wagtailadmin_home' %}">Metro Login</a></li> |
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.
❤️
P.s., I pushed an updated fixture omitting the OCD models – it should only contain content managed by the CMS! |
@hancush public comment changes reverted and notices added to userbar! |
I want to look at this with fresh eyes in January, but for now, it looks great, @xmedr! |
Overview
This branch replaces the hard coded public comment instructions with wagtail-editable objects from an
EventNotice
model. These objects will render according to conditions that Metro can select for each notice, and those condition options came from the conditions that were present within theevent_header_*_public_comment
templates. There are two sets of conditions: the status of the broadcast, and its public comment setting.I've also updated the fixtures with a set of event notices that match what metro currently has on the site.
Testing Instructions
** Note: This PR requires a database migration, the review app will not work and this will have to be tested locally.
load_content
management commandlocalhost:8001/cms