-
-
Notifications
You must be signed in to change notification settings - Fork 786
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
Removed Yujin Chang from Project Profile: Lucky Parking #5901
Removed Yujin Chang from Project Profile: Lucky Parking #5901
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.
Note that CONTRIBUTING.md cannot previewed locally; rather it should be previewed at this URL:
|
For availability and ETA for this issue: |
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 @jleung7158!
Great job overall with this pull request!
- The branching was done correctly
- Issue number was listed
- The PR title is descriptive of the changes
- Changes were made correctly in the code
- Changes appear correctly on the site. Yujin Chang has been removed.
- Relevant images were included to show visual changes
- The PR request clearly states what was updated
- The PR request states why the changes are being made
I do have one request to remove changes that are not included in the issue. Line 65 of _projects/expunge-assist.md has two backticks added that shouldn't appear. It should read Slack
instead of Slack''
. I believe this is likely due to not pulling the most current changes into your local gh-pages before branching. If I remember correctly, you were working in that file for your last issue.
- Please pull the hackforla/website gh-pages into your feature branch and then push it back to your fork. The PR should automatically include those changes once they are pushed. You can verify the changes in the
Files changed
tab of the pull request to make sure only the intended changes are included.
Once it's updated, remember to click the circle with the arrows next to my user name in the reviewers section. Thanks for taking the time to contribute to the website!
Thank you @LRenDO ! I have tried doing what you suggested, and I hope it was done correctly. It's strange because I did do a git pull upstream and push onto my local branch before making the new feature branch, but I guess it is out of sync. I will double check the CONTRIBUTING.md to see what I can do, though I am also open to any suggestions you may have! |
Thanks @jleung7158! It looks like it fixed the Slack issue in expunge-assist.md, but looking at the I'm also not sure then how the expunge-assist.md change remained in there. I am happy to jump on Slack with you and look at what you've got and your process and see if we can figure out what happened and/or prevent similar hiccups in the future. |
Alright @LRenDO I did another push/pull as late as I could, hopefully it will be good this time! |
Hi @jleung7158! Thanks for updating again. Unfortunately, it looks like that didn't work. Removing Sam's info manually is probably the fastest way to fix the issue and the easiest to explain. Please delete the following lines (64-69) and push it back to your fork.
Again, you can verify only your intended changes have been made by clicking the |
availability: Sunday, Monday, Tuesday NZ time |
Hello @jleung7158 thank you for taking up this issue! From my review, I can see the PR was correctly done with the correct branch and you have successfully removed Yujin's information as requested in the issue. Code after removal of Yujin's information looks clean and tidy. Well done! Good job on showing the before and after changes visually. I will wait until you delete lines 64-69 as requested by @LRenDO and then I will review it again so I can approve your PR. |
I have removed the info manually as suggested! Ready for review @LRenDO @JanineSooThow |
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.
@jleung7158, thanks for making the updates! Looks like it's all set on my end!
Hello @jleung7158 thank you for the changes! Just had a check and it looks all good! :D Awesome job! Will approve this PR |
Fixes #5801
What changes did you make?
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