-
Notifications
You must be signed in to change notification settings - Fork 153
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
Social shares on Buyer's Guide product pages #1975
Conversation
Amazing! You got it to pull the product name and it's creepiness! |
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.
👏
@@ -159,6 +160,18 @@ export default class CreepVote extends React.Component { | |||
* @returns {jsx} What users see when they have voted on this product. | |||
*/ | |||
renderDidVote(){ | |||
let numGroups = 5; |
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.
Pomax landed a PR that has the creepiness groups as a constant- can we leverage that so we're not replicating it here?
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.
@alanmoo can you point me where that variable is? I can't find it in creep-vote.jsx
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.
https://github.com/mozilla/foundation.mozilla.org/pull/1967/files (creepiness-labels.js)
let userVoteGroup = Math.floor(this.state.creepiness/(100/NUM_GROUPS)); | ||
let creepType; | ||
|
||
if (userVoteGroup < Math.floor(NUM_GROUPS/2)) { // lower half groups |
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.
Can't this be something like
creepType = CREEPINESS_LABELS[userVoteGroup]
?
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.
Nah, it's not 1-1. We only have 3 versions of keyword we wanna use in social share.
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.
version depending on a users vote: "CREEPY" (first two bars), "NOT REALLY CREEPY" (middle), "NOT CREEPY" (last two bars)]
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.
We can do what Pomax did to make things consistent.
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.
k I'll update - @kristinashu should I make the label all caps in social share? e.g., SOMEWHAT CREEPY, A LITTLE CREEPY etc
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.
lol yeah uppercase is great, thank 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.
Unless that's more complicated, then lowercase is cool too!
Done! @alanmoo |
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.
There we go!
Related to #1899
Covers
Based on specs in #1899 (comment) but we might want to update the pre-populated text a bit though... /cc @kristinashu