Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2011 +/- ##
==========================================
- Coverage 43.22% 43.05% -0.17%
==========================================
Files 575 576 +1
Lines 24747 24761 +14
Branches 7343 8222 +879
==========================================
- Hits 10696 10660 -36
- Misses 14002 14052 +50
Partials 49 49 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Looks great! I would rather use the property tags component for handling this as we have issues with these all the time and keeping all the logic in one place would be helpful, though.
Also, tiny aside, but perhaps we should add mw-max-content to the pill classnames. Two of them are only one word, so this should only affect no-filter, but I guess for consistency we could do it for all of them.
This'd look like this for the three tags:
{tags?.includes("nofilter") && isStaff(user) && <span
className="pill-tag-outline mw-max-content"
>NO-FILTER</span>}| {title && <CardTitle className="mb-0 pod-title" id={`event-title-${id}`}><h5>{title}</h5></CardTitle>} | ||
| <CardTitle className="mb-0 pod-title d-flex align-items-baseline" id={`event-title-${id}`}> | ||
| {title && <h5 className="mb-0 me-2">{title}</h5>} | ||
| {event.tags?.includes("nofilter") && isStaff(user) && <span className="pill-tag-outline">NO-FILTER</span>} |
There was a problem hiding this comment.
Given that events aren't likely to be superseded, deprecated, or have accessibility tags, can we use <QuestionPropertyTags tags={...} /> here just so that all the logic for these tags is always in the same place?
(maybe we should rename the component to <ContentPropertyTags /> given it's being much more widely used now...)
There was a problem hiding this comment.
Good call. I'm slightly concerned that the /questions/${supersededBy} could become problematic if we use this too widely and somebody tries to supersede a concept (?), but I agree that this is neater than duplicating the logic everywhere.
There was a problem hiding this comment.
Ooh, good point. The parent object typically knows what content type it is, so we could pass the /questions/ / /concept/ prefix in there. I might look at fixing this in a separate PR. :)
No description provided.