-
-
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
Integrate Donation GIF component into Join Us donation include #4926
Integrate Donation GIF component into Join Us donation include #4926
Conversation
…tor associated SCSS on page and component
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.
|
@roslynwythe & @rdhmdhl when you guys get a chance please put down your eta? |
I will have this reviewed by tomorrow night. |
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.
From my end, checking the code and looking at the Figma design and the local host version, it does look different from the current hack4la website, but you said you made these visual changes to accommodate the drop-down menu, so if that is true, I think everything looks good and the other code rendered correctly. You did a great job making these design decisions yourself.
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.
Nice job here.
I've reviewed these changes in my browser for mobile and desktop. It looks like the gif component is linked properly in the join-donate-card.html file, and the previous elements inside the join-us-donation div are removed. Your minor styling changes look good. All of the un-used classes in _join-us.css are removed.
My only callout for the merge team is that the the new component, donate-gif-text.html, re-ordered the gif to be at the bottom of the ordered list, instead of at the top like the website currently is. Not sure if this was intensional, but it does look better this way.
Review ETA: 7/13/2023 |
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.
Hey @adrianang the changes from mg tags to h3 was done well and thr same for the css and the scss properties changes as well
Nicely done @adrianang! |
…tor associated SCSS on page and component (hackforla#4926)
Fixes #4507
What changes did you make?
<h3>
heading as a sibling of the component, within<div class="join-us-donation">
.<h3>
in the integration of the component in related issue Create donation gif component 4439 #4650 (implementing the component into the Donate page donation include).pages/join-donate-card.html
with the new donation GIF component that encompasses the same content (body, image/GIF, footer).join-us-donation
div
now usesflex-direction: column
to accommodate structure using the new component, also matching the previously merged implementation on/donate
.donation-footer
(48px) as the previous amount (20px) would cause the first line of the footer text to overlap with the last line of ordered list item 2, especially at viewport widths from 767-790px (likely due to additional padding within the subsection the component is situated in)_sass/components/_join-us.scss
was refactored to delete styles that are no longer used, and to separate component styles from target page styles (mostly deleting styles using/related to.join-us-donation-body
)_sass/components/_donate-components.scss
was refactored to remove margins/paddings that caused component body text to be misaligned/extending past parent container boundaries, as well as adding some missing spaces within styles (new line 106, 113)Why did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
Visuals after changes are applied
Notes:
-Header is intentionally different from before change to (1) separate the header from the component internal code, and (2) ensure consistency with the implementation of the header in the integrated component on
/donate
.-'100% of donation proceeds...' line is below the GIF per action item 4 in the linked issue.