-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Display sponsors in categories #1466
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
Conversation
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.
Haven't tested this out locally yet, but just mentioned some semantics and component api things I noticed. Once you have a chance to review (and make any changes) ping me and I'll test locally. I get the general idea though, seems reasonable.
components/splash/splash.jsx
Outdated
<h2>Sponsors</h2> | ||
<Support number={ 100 } type="sponsors" /> | ||
<h2>Platin Sponsors</h2> | ||
<Support size="platin" minimum={ 50000 } type="sponsors" /> |
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.
Should this be "Platinum Sponsors"?
components/splash/splash.jsx
Outdated
<Support size="gold" minimum={ 10000 } maximum={ 50000 } type="sponsors" /> | ||
|
||
<h2>Silber Sponsors</h2> | ||
<Support size="silber" minimum={ 2000 } maximum={ 10000 } type="sponsors" /> |
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.
Should this be "Silver Sponsors"?
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 the answer is yes to both of these then we should update the semantics throughout the whole PR (e.g. props, css classes, etc.).
border-radius: 50%; | ||
border: 2px solid rgb(112, 202, 10); | ||
border: 1px solid rgb(112, 202, 10); |
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.
Just gauging from the screenshot, it seems some of these style changes might have caused the normal backers display to be thrown off. I'll check locally as well but is there a reason you changed the "normal" backers style? They look pretty good to me as is in Chrome.
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 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 screenshot is on 50% browser zoom. On 100% it's looks normal.
I reduced the size of the backers by 50% to match the size of the bronze sponsors.
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'll just double check this locally then when testing to make sure everything looks ok.
components/support/support.jsx
Outdated
@@ -4,25 +4,34 @@ import './support-style'; | |||
|
|||
export default class Support extends React.Component { | |||
render() { | |||
let { number, type } = this.props; | |||
let { minimum, maximum, size, type } = this.props; |
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 might rename the size
prop to level
or rank
or something similar. "Size" sounds more like count to display or image size to me than their rank based on donation amount. Also, maybe instead of minimum
and maximum
being props along with level
, we could have that data within the component file so only level
needs to be passed.
return ( | ||
<div className="support"> | ||
{ | ||
supporters.slice(0, number).map((supporter, index) => ( |
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.
Just want to make sure this is intentional -- we don't want to limit the backers or supporters displayed 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.
Yep it was intentional.
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.
👍
It seems the build error was a 404'd backer image... not sure if that was just a temporary outage or caused by these changes. I restarted the build to see if it's reproducible. |
Platinum, Gold, Silver, Bronze Logos have different sizes Size of Backer logos reduced
0ef4689
to
070c315
Compare
CI seem to be correct with some URLs being broken: Adam Howard Dana Woodman That's the data we receive from opencollective API. cc @xdamman |
Yeah just weird it's only being thrown on this PR -- ah I guess because more backers are now being shown. |
Test locally, looks good. Working on the build failures, it looks like the change in class name might have thrown off our |
Love this! Yeah some backers don't always provide a valid avatar :-/ The only thing that I'd add to this is an explanation of what makes a platinum/gold/silver sponsor. One of the thing you can do is to redirect the button become a sponsor to a custom URL e.g. https://opencollective.com/webpack/donate/500/month/silver%20sponsor See also our new buttons on https://opencollective.com/widgets |
Yeah not sure there's much you can do. I updated our link testing to ignore them which should be fine for now.
Yes I agree, I think a small description under each section is probably warranted. @sokra we can probably just add a
Awesome, thanks! |
Categories: Platin, Gold, Silber, Bronze
Logos have different sizes