-
-
Notifications
You must be signed in to change notification settings - Fork 7
feat: default button #124
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
feat: default button #124
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/everydotorg/donate-button/242t6TzYQoiDW8xu7Knuix2fVwJu |
README.md
Outdated
| For nonprofits — the simplest way to give your supporters a beautiful donation experience. This button opens a donation flow through [Every.org](https://www.every.org/nonprofits). | ||
|
|
||
| See live example here: https://assets.every.org/donate-button-v2/ | ||
| See this in production helping raise funds at https://ffungi.org/eng/ |
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.
cool, but think we should still provide a demo version of it to showcase all the options - looks like the assets.every.org one is broken for some reason (split panel isn't working) so i'll look into that.
| </script> | ||
| ``` | ||
| ### Customize the button | ||
| The button accepts an object with the following properties to configure the styles: |
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.
Eventually we can just only document this style and deprecate the DIY version, once we get to a copy-pasta-able "build your donate button" type of flow.
0adc366 to
f224e66
Compare
| const getCharityName = ( | ||
| widgetOptions: Partial<DonateButtonOptions> | ||
| ): string => { | ||
| const genericFoundation = 'your-foundation'; |
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.
hm would be nice if we kept this kind of hard-coding all in one place instead of spreading it across different parts of the app
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.
Yes, what I realized doing this is that if the user wants to use the onSubmit function instead of the params mode we will not have access to the charity name. Probably we should move that property up on the options object.
| const hrefUrl = `https://www.every.org/${charity}/donate`; | ||
|
|
||
| if (div) { | ||
| import('./components/GenericButton') |
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.
Given that this is the default i'm OK with not putting it behind a loader and just always loading it. It's negligible bundle size and would result in a faster experience to not have to chain HTTP calls.
| const div = document.querySelector(selector); | ||
|
|
||
| const charity = getCharityName(widgetOptions); | ||
| const hrefUrl = `https://www.every.org/${charity}/donate`; |
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.
Also would be nice to re-use the url creation from what we actually use in the real donate button. If we do it like this for instance, it loses important information as a fallback for the event handler code not working, for instance we allow tying a "partner webhook token" that allows partners to receive updates when people attempt a donation; by not using the real URL, that query param goes missing, so this "link fallback" would not pass information back to the donor website.
I think we can just use the same URL construction function, and omit stuff that hasn't been selected like frequency and what not. A pro of this is that it makes the ability to use a staging environment simpler (since we'd only need to augment that function).
|
I did some rebasing so you will probably need to overwrite your local history to pull the changes @martinbianchi |
osdiab
left a comment
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.
see the comments for requested changes.
osdiab
left a comment
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 update docs again and then merge, thanks!
closes #98
closes #122
I based this branch with the changes of #120 and #119 because we have some useful code there and we will avoid conflicts when those PRs will be merged.