-
-
Notifications
You must be signed in to change notification settings - Fork 777
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
Create donation gif component 4439 #4650
Create donation gif component 4439 #4650
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
Availability: 5/11 2 - 4pm |
Availability: 10AM-3PM |
Hi @bphan002 I apologize for the delay in reviewing. I need to update my ETA: Availability: 5/15 11 -2pm, 5/16 2 -5 pm |
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.
Hi @bphan002 Thank you for great work on this PR. The PR is well formed and the refactored HTML/CSS looks great so far., however I need to ask for a change. I apologize that this was not mentioned explicitly but the intention is that the component include the sentence "Hack for LA takes donations through Code for America". This was depicted in both the "component elements" screenshot and the "Final appearance" screenshots (under Resources). You could grab the html from this page: https://www.hackforla.org/about/ (3rd card from the bottom).
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.
Hi @bphan002 ,
As <h3>
and .donate-title
is moved outside the component, styling in line 78 and line 110 in _donate-components.scss
could be moved to _donate.scss
Also, the text inside the red circle is not included in the new version.
@Thinking-Panda Thank you for catching that. I'm not sure if you are able to chime in on this question I left for Roslyn. Is it okay if I just make the whole component flex then? |
Changing it to flex will move the text "100% of donation..." on top of the donation gif in the mobile viewport |
ETA- 5/19/23 |
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.
@bphan002 , a couple of things that I noticed.
- The
<h3>
indonate-card.html
should be "Make a Donation" _donate.scss
could have a clean up of lines 11-14
Thank you @bphan002 for resolving the merge conflict. Review ETA: EOD 5/22/23 |
Extending my ETA to 5/23 |
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.
@bphan002 , changes to _projects/access-the-data.md
are not mentioned in this issue. Rest look good.
Thank you so much for your patience. I have corrected this. |
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 done @bphan002 . Thank you for your work!
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.
Hi @bphan002 great work so far on this issue. Please adjust the styles to match the "final appearance" screenshots a bit more closely.
For desktop screen sizes, please align the sentence "Hack for LA takes donations.." with the top of the form box, and allow more whitespace between each line of text, and italicise the sentence "100% proceeds go to ..". Also the size of that italicized text should be reduced. Make sure there is enough spacing so the text doesn't get too close around 780 px.
In mobile, again please add some spacing between the lines of text, and the text "*100% of proceeds" should be left-aligned.
Hi @roslynwythe I tried the text-align:left , but it had no effect so I put a - margin. Hope that is okay. |
Hi @bphan002 thank you for making the requested changes. They look really good. Please update the "Implementation After" screenshot to reflect the latest appearance of the PR. |
Hi Roslyn, I just updated the "Implementation After" desktop and mobile screenshots. Thanks |
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.
@bphan002 , Thank you for your patience! The italics for "100% of ..." is applied only when viewport is min-width: 768px and smaller viewports have a regular font. Applying the italics style to all viewports would achieve our goal.
All set |
ETA: 5/31/23 |
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.
Excellent work. Thak you @bphan002 ! Approved.
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.
Thank you so much @bphan002 for making those changes. Please update the "Implementation After" desktop view.
Apologies for missing this and thank you again for your patience. I just updated the desktop view. |
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.
Thank you @bphan002 for great work on this issue. The code changes are correct and clean, and work perfectly in the browser, the PR is well-formed and you decribed your work, and you were responsive to requests for changes.
Fixes #4439
What changes did you make and why did you make them ?
Implantation Before
Desktop
Mobile
Implementation After
Desktop
Mobile