Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Donation modal #2972

Merged
merged 32 commits into from
Sep 21, 2023
Merged

Add Donation modal #2972

merged 32 commits into from
Sep 21, 2023

Conversation

dem4ron
Copy link
Member

@dem4ron dem4ron commented Sep 22, 2022 •
edited
Loading

Screenshot 2023-09-21 at 11 17 21

iHiD
iHiD reviewed Sep 22, 2022
View reviewed changes
app/css/ui-kit/buttons.css Outdated Show resolved Hide resolved
iHiD
iHiD reviewed Sep 22, 2022
View reviewed changes
app/css/ui-kit/filters.css Outdated

.filter-shadow-huge {
-webkit-filter: drop-shadow(0px 4px 128px rgba(79, 114, 205, 0.8));
filter: drop-shadow(0px 4px 128px rgba(79, 114, 205, 0.8));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do this via tailwind, which removes the need for the extra webkit variant: https://tailwindcss.com/docs/drop-shadow#arbitrary-values

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dem4ron

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually unused. It might have been used at some point in this PR, but upon doing a grep, I couldn't find any references to it. I've removed it.

@dem4ron
Copy link
Member Author

dem4ron commented Sep 22, 2022

@iHiD about modal a11y:
It seems to be a bad idea to have the same ids multiple times, but these elements get replaced, and there is only one unique id present in the DOM, and I don't really know how it could be easily bypassed in this structure. What do you think about this?
I think I'll summon @SleeplessByte :)

@SleeplessByte
Copy link
Member

  • If the question is: should you ever have two or more elements with the same ID, the answer is no, which I think you also say (unique).
  • If the question is: is it okay to remove an element with an ID to later add an element with that same ID, even if the added element isn't the same as the removed one: that's fine. I think this is the case given your comment about there only being one instance per ID in the DOM at the same time.

@iHiD
Copy link
Member

iHiD commented Feb 16, 2023

@dem4ron @ErikSchierboom Can we get this green pls (might just need a rebase). Then can one of you update the OP so I know what I need to do get it over the line. Thanks.

@ErikSchierboom
Copy link
Member

My latest commit should have fixed the failing test.
@dem4ron do you know what the status is on the client-side? I think the modal was not yet shown, was it?

@dem4ron dem4ron force-pushed the add-donation-option-after-testimonial branch 2 times, most recently from 601ee96 to 703996d Compare September 18, 2023 10:37
dem4ron and others added 3 commits September 19, 2023 15:08
@dem4ron
Reorganise MentoringSession's donation data (#6206)
dd347cc 
* Reorganise MentoringSession's donation data

* Update mentoring_session_test

* Pass down camelized data

* Pass donation data down to sidepanel modal initiator
@iHiD
Improve layout and copy
1d983a8
@dem4ron
Clean up DonationStep.tsx
777f395
@iHiD
Copy link
Member

iHiD commented Sep 19, 2023

@dem4ron This just needs the "don't close when clicking the background" thing setting and then I think your bit is done.

@iHiD
Copy link
Member

iHiD commented Sep 19, 2023

And also just check my comments.

dem4ron and others added 2 commits September 19, 2023 19:22
@dem4ron
Sync
8643f2b
@dem4ron
Further donation modal tweaks (#6214)
f310167 
* Change button text, remove static booleans

* Change default values

* Update celebration text

* Update celebration step

* Update urls

* Use confirmParamsReturnUrl everywhere

* Add successfulDonationStep

* Update xstate

* Change width of SuccessfulDonationStep's container modal

* Change nyan cat height

* Change continue text

* Apply suggestions from code review

* Update test

* Remove recaptcha sitekey

* Change text in test

* Fix more tests
ErikSchierboom
ErikSchierboom reviewed Sep 21, 2023
View reviewed changes
.github/labels.yml Outdated Show resolved Hide resolved
CODE_OF_CONDUCT.md Outdated Show resolved Hide resolved
app/helpers/react_components/donations/footer_form.rb Outdated
@@ -17,7 +17,7 @@ def to_s
recaptcha_site_key: ENV.fetch('RECAPTCHA_SITE_KEY', Exercism.secrets.recaptcha_site_key),
links: {
settings: Exercism::Routes.donations_settings_url,
donate: Exercism::Routes.donate_url
confirm_params_return_url: Exercism::Routes.donate_url
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure about the name. What is this link used for? We usually don't use a _url suffix

Copy link
Member Author

@dem4ron dem4ron Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes into stripe.confirmPayment.options.confirmParams.[returnUrl | return_url]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably rename it slightly to something like donate_return_url.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm ok with the confirm_params_return bit as it's in the scope of the form, but agree it probably shouldn't have the _url prefix. Pragmatically I don't care if its hard to change it, but if it's just a couple of renames it would be better to be consistent.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but the reason I chose this name is because this is exactly what the API accepts. so it is pretty specific.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the reason I was suggesting donate_return_url (actually, redirect_to_after_donation or something like that might be better) is because it isn't coupled to Stripe. I feel that's an implementation detail 🤷

app/javascript/components/modals/student/FinishMentorDiscussionModal.tsx Outdated Show resolved Hide resolved
app/javascript/components/types.ts Outdated Show resolved Hide resolved
app/models/user.rb Show resolved Hide resolved
dem4ron and others added 3 commits September 21, 2023 10:27
@dem4ron dem4ron marked this pull request as ready for review September 21, 2023 09:16
@iHiD iHiD merged commit 85f2891 into main Sep 21, 2023
@iHiD iHiD deleted the add-donation-option-after-testimonial branch September 21, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iHiD iHiD iHiD left review comments

@ErikSchierboom ErikSchierboom ErikSchierboom left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dem4ron @SleeplessByte @iHiD @ErikSchierboom