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.

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

fix: allow donate modal to dynamically change for devices #945

Merged
merged 6 commits into from
Dec 1, 2024

Conversation

yontank
Copy link
Collaborator

@yontank yontank commented Nov 24, 2024

Description

Dynamically changes DonationModal to support (hopefully) most devices,
as written in #942

screenshots

S

@yontank yontank changed the title Fix-collapsing-text-on-donate-modal bugfix: allow donate modal to dynamically change for devices Nov 24, 2024
@NoamGaash NoamGaash changed the title bugfix: allow donate modal to dynamically change for devices fix: allow donate modal to dynamically change for devices Nov 25, 2024
@NoamGaash
Copy link
Member

NoamGaash commented Nov 25, 2024

It looks great, thank you!
my thoughts -

  1. merge from main to get the latest version (with the right color)
  2. I think the desktop version should not ne aligned to the center - what do you think?

When you feel it's ready, please mark the pull request as "ready for review"

@yontank
Copy link
Collaborator Author

yontank commented Nov 26, 2024

yeah I agree, setting it to the left looks much better,

got 2 questions:

  1. Is it okay to use CSS styles right inside the JSX tags? for example
  2. about the jgive image, is it okay that it's moved to the left?

@NoamGaash
Copy link
Member

  1. it's ok, but it's better to use scss or styled components
  2. sure 🙏

by the way, I think that we should add some tests to see how things look in English (not just this modal)

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

I think it's a great improvement - so please feel free to merge if you feel comfortable with that code, and open another PR if you want to make more progress 🙏

@yontank
Copy link
Collaborator Author

yontank commented Nov 30, 2024

Hey, Sure thing! I thought about creating the tests and then merging, but it's probably gonna take some time since I'm new PlayWright 😢

@yontank yontank marked this pull request as ready for review December 1, 2024 04:10
@yontank yontank merged commit 670df7a into main Dec 1, 2024
18 checks passed
@yontank yontank deleted the fix-collapsing-text-on-donate-modal branch December 1, 2024 08:18
@NoamGaash NoamGaash linked an issue Dec 2, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

css | fix donation modal for mobile devices
2 participants