-
-
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 project page header 2355 #3087
Update project page header 2355 #3087
Conversation
Blocker: Previous commits from a different issue somehow got grouped into this pull request. I'm not sure what is the best way to go about fixing this conflict. |
ETA: End of the day on 5/06 |
hey @Jaretzbalba I'll take a look sometime today/tomorrow but can you reference the other issue that changes somehow got lumped in from? I'm assuming its these 3 that have no place in this PR:
Avail: 1-2hrs |
@Sparky-code The issue that got lumped into this PR was issue #3022/ PR #3043. Yes, the files that shouldn't be included in this PR are: _data/internal/credits/tools/so-go-survey.yml |
@Jaretzbalba Try a git revert on the commit that shouldn't be in this pr and push to the branch Also, after that, I would recommend updating that branch with the latest changes from Hack for LA's gh-pages. |
@SAUMILDHANKAR @tamara-snyder @answebdev @Sparky-code There is something weird going on with all the automations/checks not running, which there are usually 3 that run. (See "Checks" tab for this pr.) I'm not sure if it has something to do with the merge conflicts. However, I manually added the labels to help developers looking to review prs to know about the pr/issue by the labels. |
This reverts commit 77e3ba0.
@JessicaLucindaCheng The git revert seems to have fixed the file issue. However, I can't seem to get this current branch to update with the latest changes from gh-pages. When I'm on the branch I'll check upstream and it claims it is already up to date; however, I can clearly see that files are missing from recent merges and my branch on GitHub even says that it's behind. |
The blocker was resolved during the team meeting on 5/10/22. @JessicaLucindaCheng recommended merging the upstream-gh-pages to my topic branch to have the most up-to-date files. However, since my gh-pages are a few commits ahead, I will need to refork this repo. |
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.
Content changes look good on both mobile and and desktop. Links in the callouts work as expected and CSS is clean and works well. Happy to see the files issue was resolved as well.
Great job!
Availability: 1 hour Edit: Apologies, I ran out of time to look at this yesterday. Updated review ETA: Thursday, May 19 |
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 @Jaretzbalba, well done on this! The desktop and mobile versions look great. I left a couple of comments, but the main one is just that a tablet query may need to be added in order to accommodate about 40px of width where the paragraph spills off the page. You'll find a screenshot of the problem in my comment. Thanks for working on this! Feel free to request another review when you're ready.
Avail: 2hrs |
Sorry for the delay - Reviewing today |
@Jaretzbalba So I see that you made the correct changes, addressed the issue with tablet sizing and the resulting complication with the page to the unfinished 'about us' page. |
@Sparky-code We actually did discuss it in the meeting. Bonnie said to merge it onto the feature-homepage-launch branch. So, don't hide anything within a comment. @Jaretzbalba So, now you will need to do the following:
|
@JessicaLucindaCheng Just to make sure I do this correctly should the {FEATURE BRANCH NAME} in the steps provided be |
|
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 see that the branch was updated to reflect the discussion had in the comments regarding that.
Changes appear to have been resolved
ETA: End of day 6/20 |
git checkout -b feature-homepage-launch upstream/feature-homepage-launch |
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.
@Jaretzbalba Changes all look good locally and the links work. I see that you have pushed your changes to the correct feature branch as well. Overall, very nice work, Jaret. Thank you.
Fixes #2355
What changes did you make and why did you make them?
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