-
Notifications
You must be signed in to change notification settings - Fork 47
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
Add donation links #96
base: master
Are you sure you want to change the base?
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.
This is an excellent pull request. You figured out a lot of stuff and made it work. It looks really polished and I don't have any technical concerns.
@@ -660,6 +660,36 @@ iframe.js { | |||
background: @brand-color; | |||
} | |||
|
|||
&.donations { | |||
background: @brand-color; | |||
animation: rotate-color 30s linear infinite; |
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 find the color cycling a bit distracting, but my wife loves it. That's the extent of my user testing so far. Could you tweak the transition time from 30s to 180s?
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.
Well... I'd say one is supposed to handle a notice swiftly... But sure.
margin-top: -50px; | ||
height: 50px; | ||
|
||
a { |
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 link looks better with font-weight: bold and color: #333 (or something like that).
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 pure black color could be considered a subtle form of highlighting... if the main text above were off-black. Will do.
@@ -304,6 +306,15 @@ export default React.createClass({ | |||
</div>} | |||
{pmNotices.map(pm => <PMNotice key={pm.get('kind') + pm.get('id')} pmId={pm.get('id')} nick={pm.get('nick')} kind={pm.get('kind')} />) } | |||
{this.state.update.get('ready') && <FastButton className="update-button" onClick={update.perform}><p>update ready<em>{Heim.isTouch ? 'tap' : 'click'} to reload</em></p></FastButton>} | |||
{this.state.donations.eligible && this.state.donations.url && this.state.ui.notices.has('donations') && <div className="notice dark donations"> | |||
<div className="content"> | |||
<span className="title">euphoria maintenance and development is enabled by</span> |
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 find the mix of sentence and button in the notification to be confusing to parse when I first see it. The button calls my attention first, and then I read the label. Maybe "euphoria maintenance and development depends on you [support our patreon]"?
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.
Will do so.
Coded closely after the notification popup; one should probably dismiss this one globally.
The URL has to be configured via the HEIM_DONATION_URL environment; the notice will not show up at all otherwise.
- The HEIM_DONATION_URL variable was not properly propagated into the build. - The donation store would race with the localStorage one for the latter's load event.
- The front page one gets bold text and a lighter color. - The in-room one does not embed the button into the sentence, and the hue rotation is slower.
936bc24
to
392be76
Compare
The modifications — aside from the static page footer spacing — remain invisible unless the
HEIM_DONATION_URL
environment variable is set appropriately (i.e. tohttps://patreon.com/euphoriachat
for euphoria.io).The branch is fully functioning and can be merged if its behavior is acceptable:
Frontend tests pass.