-
Notifications
You must be signed in to change notification settings - Fork 47
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
Increase Sponsor Icon Size for Mobile Readability #612
base: develop
Are you sure you want to change the base?
Conversation
@mai-repo Thanks for tackling this! The icons still look quite small to me (although they are bigger than before). I think to make them more responsive, it would be good to remove some of the extra padding around the outside and make them larger, taking up two icons per row. Additionally, I see that you modified a class called "twenty-percent-width" and made the value "30%". I would avoid doing this, as the class "twenty-percent-width" should always remain 20%. You can add a class to these icons to change in this case, as "twenty-percent-width" may be used across the entire site and we wouldn't want that changing to 30 for things other than these icons. Lastly, I would suggest naming your branch with the issue id in the future so we can easily track work by branch. Just let me know if you have any questions around these suggestions! |
@monikkaelyse I implemented your suggestions, and you can see the changes here: 052a506. |
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.
Great job adding the additional class for mobile view and the updated icons are so much more accessible!
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.
The icons look great now ! Nice work!
Just a suggestion below about moving to either 1 class or 2 (would avoid having a class applied to a non-mobile div named "mobile"). The nice thing about media queries is that you can just use one class rather than adding more.
templates/sponsor.html
Outdated
@@ -147,21 +147,21 @@ <h1 class="blue-h1 centered">Join our hiring partners!</h1> | |||
<img | |||
src="{{ url_for('static', filename='img/pantheon-logo-min.jpg') }}" | |||
alt="Pantheon.io logo" | |||
class="twenty-percent-width margin-25" | |||
class="twenty-percent-width margin-25 sponsor-mobile-size" |
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.
I would suggest having just one class on here defining the width since it changes responsively, maybe replace "twenty-percent-width" and "sponsor-mobile-size" with "sponsor-logo" and then set it to
{width: 20%} then in the mobile css have it change to 33%.
Optionally, you could also remove the "margin-25" class, and just put that margin into your "sponsor-logo" class.
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.
Thanks @monikkaelyse, you should have an update on this tomorrow!
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.
@monikkaelyse please see the changes made here commit d7178e3
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.
Great work @mai-repo !
Description
This PR resolves the issue of sponsor images appearing too small on mobile devices by adjusting the styling to improve legibility while maintaining an optimal layout.
Changes Made
File Changes
style.scss
Before
After
Objective
Ensuring that sponsor images are easily readable on mobile enhances the user experience, particularly for those viewing hiring partner information.
Next Steps
Reviewers: @daaimah123, @mx-ruthie, and @ChasVanDav please check the updated styling in the
style.scss
to confirm the icons are now legible across all relevant pages.