-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
Via modtools a Guardian-employee profile can be marked as a contributor (eg a freelancer). In frontend we marked comments from contributors with an associated badge. This change adds the equiv functionality. Note that frontend assumes we show either Staff or Contributor badges, not both.
Size Change: +4.8 kB (+2%) Total Size: 218 kB
|
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.
Nice! Always happy to see Discussion getting some ❤️ !
Just a few comments, nothing earth shattering and I'm happy to leave them with you to do as you see fit. On thing though, it'd be ace to get a screenshot in the description showing how this new badge looks 🙏 Thanks!
(obj) => obj['name'] === 'Staff', | ||
); | ||
|
||
const showPickBadge = comment.status !== 'blocked' && isHighlighted; | ||
|
||
// A contributor could be e.g. a freelancer that commonly comments on articles. | ||
// In frontend we check/display the Staff badge else we check/display the |
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.
If this Frontend code is relevant here should we link to it?
src/components/Comment/Comment.tsx
Outdated
@@ -455,9 +460,11 @@ export const Comment = ({ | |||
</div> | |||
</Row> | |||
<Row> | |||
{showStaffbadge ? ( | |||
{showStaffBadge || showContributorBadge ? ( |
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 feels a bit like these two bits of logic are fighting each other. Maybe we can break this apart and simply have
{showStaffBadge && <GuardianStaff role='Staff' />}
{showContributorBadge && <GuardianStaff role='Contributor' />}
?
If we explicitly want to prevent both badges showing at the same time you could
{showStaffBadge && <GuardianStaff role='Staff' />}
{showContributorBadge && !showStaffBadge && <GuardianStaff role='Contributor' />}
not sure if this extra check is needed or not. Ideally the server would decide this type of thing and the rendering layer just does what it is asked to do
src/components/Badges/Badges.tsx
Outdated
@@ -34,7 +34,7 @@ const guardianPickLabel = css` | |||
color: ${neutral[7]}; | |||
`; | |||
|
|||
export const GuardianStaff = () => ( | |||
export const GuardianStaff = ({ role }: { role: 'Staff' | 'Contributor' }) => ( |
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.
You could also have a second component, like GuardianContributor
and then there'd be no need for the prop? I think if I were starting from scratch and these were the requirements from day one I would have done that
Fully admit that this comment is a bit opinionated and feely though so whatever feels best to you
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.
Actually, this was my initial thought too :) Looking at how the Contributor badge renders via frontend it's identical to the Staff badge, so the GuardianContributor component was a duplication with just the inner text differing. So, I tried to be tidy by giving the component a role
- might regret that choice soon!
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.
Ha, well. One reason for not using role
is that this name is used all over the place in Guardian code. Around images mainly. Incidentally, the same thing is also called weight
sometimes but that's just to keep us paying attention.
Thanks - have just figured out how to test this all locally and I need to make some changes. When I'm a bit happier with the logic I'll update this PR to include some screenshots :) |
After testing it was found that the badge array will contain an element for Contributors, similarly for Staff. Given that I have a badge array, I'm assuming I could get multiple badges and so therefore still want to handle the case where a user is marked as both Staff and Contributor, but we only show the Staff badge.
In #524 we added the Contributor badge to comments but moderators would also like the Contributor badge to appear in any comments that are the top picks.
What does this change?
Via modtools a Guardian-employee profile can be marked as a contributor (eg a freelancer).
In frontend we marked comments from contributors with an associated badge. https://github.com/guardian/frontend/blob/main/discussion/app/views/fragments/commentBadges.scala.html#L8
This change adds the equivalent functionality. Note that frontend assumes we show either Staff or Contributor badges, not both.
Liveblogs haven't been migrated to dcr yet and contributor badges in liveblogs are showing up as expected, eg https://www.theguardian.com/sport/live/2021/sep/06/county-cricket-warwickshire-v-hampshire-yorkshire-v-somerset-and-more-live#comment-151643779
Currently, for articles rendered via dcr, contributors did not have a badge:
After this change, contributors have a badge:
Why?
This issue has been reported by CP & Moderators.
Link to supporting Trello card
https://trello.com/c/J83m02bt