-
-
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
Update image location sponsors 2458 #2984
Update image location sponsors 2458 #2984
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.
|
Edit: Sorry I wasn't able to finish the review by 3/20 :( I promise I will have this reviewed by Monday 3/21 |
Ugh sorry for the delay -> |
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 Jason, after running your branch on Docker, I was able to verify that the sponsor section in the about page looks exactly the same as before.
However images at the bottom of the citizen-engagement page look slightly different. I suspect that may be because, the current Code For America logo image (/assets/images/sponsors/logo-cfa.svg) and you are trying to use (/assets/images/sponsors/code-for-america.svg) are fundamentally not same image with both having different dimension. This if you look carefully results in a slightly different layout with the Organizations We work With section in the Citizen Engagement page of your fork looking slightly expanded than it actually is. Please refer to the images I have attached below --> the first one is the current website look and the second one is from your fork.
Also at the bottom of the current home-page, the sponsor images do not look correct, they appear a little shrunk. Please refer to the images I have attached for reference --> the first image is the from the current home page and the second image is from the homepage of your fork.
The changes to the partners.yml and sponsors.yml files look correct.
I honestly don't know if you can solve this or not, I think it is worth bringing it up in the dev meeting on Tuesday(March 22nd) and hopefully we can come to a resolution.
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 @JasonY188 thanks for working on this issue! I have noticed some of the same issues that Utkarsh indicated with his review when running your code locally. I will give this another look tomorrow before the weekly meeting and add more comments if need be.
One minor comment I have unrelated to your code is about your commit messages. It's generally a good idea to make each commit message contain a little information about the change so that you can easily go back and see what was changed when. |
Thanks @luke-karis and @usaboo for reviewing my pr. After our discussion, I will try to resized the images. |
@JasonY188 Just wanted to follow up if you were able to resize the images or if it was too much hassle. Feel free to request a re-review based on your findings. Also, request you to share an update on your pre-work checklist as well. Thank you. |
@SAUMILDHANKAR I tried resizing the svg, but I'm not getting the desired result. When I tried increasing the size of the image, I am only seeing changes in the sponsors section of the about page, not in the home page. I will keep trying and provide an update on 03-29. |
Leaving a note per our discussion on Tue, Mar 29: try editing the SCSS files related to the images to try to fix sizes of the images. |
@JasonY188 Just wanted to let you know that we discussed this PR in our Dev/PM agenda. As per the discussion, it is fine to use any image format (svg, png, jpeg etc) as long as the pages look fine. So, feel free to make the changes. Please let us know if there are any updates. Thank you. |
Hi @SAUMILDHANKAR, Thanks for the update. I made a change in the _home.scss file so the svg images are a bit larger now. I will try using the previous images again and make a comparison to see which is better look. ETA: 04-11 |
ETA 04/10 by 10pm EST |
The issue I found is that the original code uses two different sets of sponsor images, one for the home page and one for the about page. Since the images are not identical, if we use the home page sponsor images, the about page images will be off and vice versa. Even adjusting the scss, I am not able to make the images look like before. On April 7th, the screenshot is the changed made on the sponsor section of home page. Today's screenshot is the change made on the sponsor's section of the about page. Please let me know which is preferred, or if you have any other suggestions. |
Re-Review: |
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 Jason, great job sticking with this, you're making great progress and the images look way better! I have two minor comments:
I noticed that there is some more spacing on the citizen engagement section you worked on.
Here is the original:
And here is what shows up when I test your changes locally
My other comment is I noticed that in the about page in the HfLA in the news section the CNN and GT images and links are swapped. Could you merge in the most recent website changes and make sure this looks good? It doesn't seem like you touched this code so I hope it's something funky with my computer and not a weird bug.
@luke-karis @JasonY188 I looked at the code for the Press and I think the order of the Press has to do with the way Jekyll process the for loop in the Liquid templating language. Thus, for now, it is fine if the order is different from how you see it locally vs on the live website for the Press section. |
Availability: 2 hours |
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.
@JasonY188 Just wanted to check in on the current status, particularly on the "Organizations We work With" section in the "Citizen Engagement" page. Like @luke-karis , I also see a difference in spacing locally - there is more spacing in yours (see second image below):
Secondly, like @luke-karis , I also noticed that in the About Page in the "HfLA in the News" section, the CNN and GT images and links are swapped. Though, according to the last comment made by @JessicaLucindaCheng , this is okay for now.
As for the Sponsors section on both the Home Page and About Page, the issues mentioned above seem to have been addressed, as they look fine for me locally now (if the images look off a little, it's because of the way I cropped them - they actually look fine online).
So basically, I'm wondering what the current status of the "Organizations We work With" section in the Citizen Engagement page is.
Hi @JasonY188 , thanks for your work on this issue. I am so sorry I haven't been as prompt about my reviews. I have been traveling over the past 2 weeks. However I do want to add something, @answebdev, @luke-karis , the sponsor section on homepage does not look for me in the deployed branch and Jason's branch, which is not what you see Adolf. This is what I see on the deployed website's home-page, This is what I see in Jason's branch's home-page running on my docker, Honestly, I feel it looks better in Jason's branch but that's for another day. I was wondering if I was doing everything right, or if we missed something. |
Thank you all for reviewing my pr. In regards to the Citizen-Engagement page in the Organization We Work With section, it's the same issue I was facing with the Sponsors section. The images being replaced are not the same in size or resolution as images replacing them. So with the new images there will be visual differences. I'll look into adding css to the single image for Code for America, but I'm not sure that is the way to go. ETA 05-10 |
@JasonY188 You could ask Design at Sunday's meeting before you make additional SCSS changes to see what they think and give you feedback. |
@usaboo I don't know why you are not able to see the same thing as me. Sometimes I have issues where I am not able to see changes locally. In this case, it has been suggested to clear the browser's cache, though I have not actually tried doing this before. My understanding is that sometimes the file watching gets messed up and you need to rebuild everything. Saumil has suggested before deleting the _site folder, deleting the .jekyll-metadata file, and then building it back up from scratch. I believe I've tried this before, and it worked for me. If using Chrome, I found that it has also worked in incognito mode. These are some things you may want to try, if it is in fact the case of not being able to see your changes locally. |
@JessicaLucindaCheng. I could not attend last Sunday's meeting and most likely cannot attend this coming Sunday's meeting. Is there an alternative? |
@JessicaLucindaCheng @SAUMILDHANKAR After Sunday's discussion, in the Citizen-Engagement page in the Organizations We Work With section, I will make SCSS changes to the Code for America image and reduce the gap between the images of the top and bottom rows to mirror the appearance of the live page. ETA 05-21 |
I'm so sorry I missed your question. If you have a question in the future and I don't respond, please Slack message me. |
ETA - Thursday 11PM PT |
ETA - 5/25/2022 (end of day) |
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.
@JasonY188 Thank you for your updates. I checked the code and the updates you made, and I was able to see the changes you made in the code locally. As for the "Organizations We work With" section in the "Citizen Engagement" page, the extra spacing that was there previously is no longer there, and it looks better now.
As for the About Page in the "HfLA in the News" section, the CNN and GT images and links are still swapped. However, as was previously commented, this is okay for now.
As for the Sponsors section on the Home Page, although it now looks like the proposed changes and matches the screenshot of the visuals after changes are applied (see initial comment in PR), it does not look the same as it currently does live, and in the original issue, the last action item states to "Ensure that the sponsors (bottom of the page) in the current homepage look the same after the changes", so I'm not exactly sure which of the two is the way it is supposed to be (unless I misunderstood something). What do you think, @JessicaLucindaCheng ?
Finally, on the About Page, the images look the same as they previously did, and the same as on the live site.
As mentioned in #2984 (comment), the homepage sponsors' images should now look like the ones on the about page. Thus, the changes that Jason made to the sponsor images on the homepage are fine. |
Good job on your consistent work on this issue @JasonY188. As far as i can see,
So everything looks good me. Maybe others could find some bug that I may have missed. |
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.
@JessicaLucindaCheng Thank you for clearing that up. I think I either missed that comment, or just understand something else, so thank you for the clarification. @JasonY188 With that cleared up, everything else looks good to me now. Great job!
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.
This looks good to me! Thanks for your continued work on this, you've done a great job pushing through and getting this done.
Used a squash and merge for this * Applied Title classes to heading elements on donate page (hackforla#2723) * applied Title classes to heading elements on donate page, repositioned first h1 element on page to align with paragraph. * changed Titlex classes to titlex, removed title classes from sass file. * fixed requested errors * Fixed the styling issues, localhost and hfla site match. * made requested changes * commiting changes so I can sync with upstream. * made requested changes * Removed Ready for milestone for Pre-work Template - Developers Reason: Tech leads should manually check the preworks before adding ready for milestone label * Update meeting data * Update contributor and language data * Update meeting data * Update contributor and language data * Update issue templates * Update meeting data * Update contributor and language data * Edited the content field and removed the type field so that redundant code is removed and the code is easier to understand (hackforla#3182) Merged hackforla#3182 into gh-pages. * Remove alt-hero field from civic-opportunity-project.md project file hackforla#2923 (hackforla#3173) * Update meeting data * Update contributor and language data * Update pre-work-template--dev.md (hackforla#3192) * Update dev prework template * Update pre-work-template---design.md (hackforla#3184) Bonnie asked to update the design template * Updated dev prework template * Update dev prework template * Update meeting data * Update contributor and language data * Update issue templates Updated estimates, time spent so far and progress report items. * Update CONTRIBUTING.md Added a note about leaving issues (other than pre-work) in In progress column until merged. * Fixed Project md file: Removing unused `alt-hero` field * Fix Alt Text Audit - Design issue template I did not make edits as part of commit fd642a4 to .github/ISSUE_TEMPLATE/alt-text-audit---design.md but for some reason changes were made. I fixed it here by editing the file to change it back. * Update meeting data * Update contributor and language data * Edited content field and removed (hackforla#3193) Merged hackforla#3193 into gh-pages. * Update CONTRIBUTING.md Added link of a filtered project board for back end good first issues. * removed unused alt-hero field (hackforla#3178) Co-authored-by: Olanrewaju-Ak <[email protected]> * Update meeting data * Update contributor and language data * Updated Project Profile Card review and update template Capitalized GitHub in the links section * Updated Pre-work Template - Developers Changed team lead to technical lead * Update issue templates (hackforla#3219) updated developer prework template * edit content field and remove type field from _data/interal/credits/resume.yml (hackforla#3218) * Update README.md * Remove alt-text field in github-issues.html, responsible-use-of-images-on-opensource-projects.html, setting-up-1password-on-opensource-projects.html, all within the _guide-pages directory (hackforla#3176) * Update meeting data * Update contributor and language data * Update meeting data * Update contributor and language data * removed unused filed (hackforla#3221) * Removed type field 2878 (hackforla#3185) * modified content field * removed type field * modified content field * removed type field * revert jekyll version * Update meeting data * Update contributor and language data * Update developer pre-work issues * Update meeting data * Update contributor and language data * Update image location sponsors 2458 (hackforla#2984) * update image location for sponsors * update image location for sponsors * update class logo img * update code in sponsors.hmtl * add scss to cfa logo in citizen-engagement page * Remove alt hero field 3215 (hackforla#3231) * Docker-compose file update * Reverted docker-compose file from '4.2.0' back to 'pages' * alt-hero field deleted * Update meeting data * Update contributor and language data * Update meeting data * Update contributor and language data * remove unused alt field from vrms.md project file (hackforla#3226) * Update meeting data * Update contributor and language data * Update meeting data * Update contributor and language data * Lucky parking 2916 (hackforla#3086) * changed the spelling from webapp to web app on line 56 * chnaged the double quotes around the links to single quotes * Update meeting data * Update meeting data * Update contributor and language data * Update dev prework issue template * Update meeting data * Update contributor and language data * Add h tags 2952 (hackforla#3237) * add civic tech overview page to assets folder * change link in markdown file * Update merge conflict * Change p tags to h tags * Removed the alt text for the 311 project card image (hackforla#3241) to adhere to WCAG. * Update meeting data * Update contributor and language data * added a new note in section 2.2 before the existing one (hackforla#3235) Co-authored-by: Olanrewaju-Ak <[email protected]> * update project profile of home unite us page (hackforla#3162) * removed Abiha Ali and added Ben Ross under leadership for home unite us page * Update meeting data * Update contributor and language data * Update meeting data * Update contributor and language data * Update meeting data * Update contributor and language data * Update meeting data * Update contributor and language data * updated calendar-time.yml (hackforla#3258) Co-authored-by: olivi <[email protected]> * Update meeting data * Update contributor and language data * Removed credits type field and renamed content to content-type in redo.yml (hackforla#3285) * Update meeting data * Update contributor and language data * Updated how 'overview' link opens for Home Unite US (hackforla#3224) * Updated how 'overview' link opens for Home Unite US * Update meeting data * Update contributor and language data * removed unused filed (hackforla#3221) * Removed type field 2878 (hackforla#3185) * modified content field * removed type field * modified content field * removed type field * revert jekyll version * Updated how 'overview' link opens for Home Unite US Co-authored-by: GitHub Actions Bot <[email protected]> Co-authored-by: Arpita <[email protected]> * changed line 4 for the content field from content:icon to content-type:image. Also removed line 11 type:icon (hackforla#3264) * Update meeting data * Update contributor and language data * Changed sdg image's alt text for access the data (hackforla#3288) * Resolve issue 2155 to add a comment reminding new issue assignees to add their ETA and availability (hackforla#2962) * changed content and type into one field: content-type (hackforla#3291) Co-authored-by: tunglinn <[email protected]> * update instructions template 2897 (hackforla#3262) Co-authored-by: olivi <[email protected]> * Update meeting data * Update contributor and language data * Update meeting data * Update contributor and language data * Updated website team members (hackforla#3298) * Changed from Content to Content-Type (hackforla#3297) * changed from content to content type * ignore * Change content to content-type * edited and removed words (hackforla#3287) Co-authored-by: Lily Arjomand <[email protected]> * Update meeting data * Update contributor and language data * Bump node-fetch from 2.6.1 to 2.6.7 in /github-actions/github-data (hackforla#3263) Bumps [node-fetch](https://github.com/node-fetch/node-fetch) from 2.6.1 to 2.6.7. - [Release notes](https://github.com/node-fetch/node-fetch/releases) - [Commits](node-fetch/node-fetch@v2.6.1...v2.6.7) --- updated-dependencies: - dependency-name: node-fetch dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * changed line 4 and deleted line 11 (hackforla#3303) Co-authored-by: Riya Aswani <[email protected]> * Update meeting data * Update contributor and language data * Changed alt text for card image (hackforla#3304) Co-authored-by: Lily Arjomand <[email protected]> * Update issue templates * Update meeting data * Update contributor and language data * changed image alt in lucky-parking.md (hackforla#3317) Co-authored-by: tunglinn <[email protected]> * Change alt text for citizen engagement html file (hackforla#3320) * changed from content to content type * change content to content-type * change content to content-type * change content to content-type * change content to content-type * change alt text line 52 * reset docker-compose Co-authored-by: Patrick McGuigan <[email protected]> Co-authored-by: GitHub Actions Bot <[email protected]> Co-authored-by: alan-zambrano <[email protected]> Co-authored-by: mmogri <[email protected]> Co-authored-by: Ava Li <[email protected]> Co-authored-by: phuonguvan <[email protected]> Co-authored-by: Saumil Dhankar <[email protected]> Co-authored-by: mchavezm <[email protected]> Co-authored-by: Akinola Olanrewaju <[email protected]> Co-authored-by: Olanrewaju-Ak <[email protected]> Co-authored-by: Bonnie Wolfe <[email protected]> Co-authored-by: Don Brower <[email protected]> Co-authored-by: Julian Smith <[email protected]> Co-authored-by: Arpita <[email protected]> Co-authored-by: Jason Yee <[email protected]> Co-authored-by: Matthew Arofin <[email protected]> Co-authored-by: Erick Odero <[email protected]> Co-authored-by: riddle015 <[email protected]> Co-authored-by: Trisha Johnson <[email protected]> Co-authored-by: Olivia Wang <[email protected]> Co-authored-by: olivi <[email protected]> Co-authored-by: Beckett OBrien <[email protected]> Co-authored-by: Utkarsh Saboo <[email protected]> Co-authored-by: Tung Lin <[email protected]> Co-authored-by: tunglinn <[email protected]> Co-authored-by: Clayton Brossia <[email protected]> Co-authored-by: Lily Arjomand <[email protected]> Co-authored-by: Lily Arjomand <[email protected]> Co-authored-by: Sarah Sanger <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Riya Aswani <[email protected]> Co-authored-by: Riya Aswani <[email protected]>
Fixes #2458
What changes did you make and why did you make them ?
-Delete the existing assets/images/sponsors directory along with its contents
-Move the file directory assets/images/about/sponsors one level up so it becomes assets/images/sponsors
-Changed the paths for: sponsors.yml, about-card-sponsors.html, partners.yml, citizen-engagement.html
-Updated the code in sponsors.html
Screenshots of Proposed Changes Of The Website (if any, please do not screen shot code changes)
Visuals before changes are applied
website.
Visuals after changes are applied