-
-
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
Update Project Profile: Lucky Parking (Remove Sijia Pitts) #5904
Update Project Profile: Lucky Parking (Remove Sijia Pitts) #5904
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:
|
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.
See inline comment regarding single/double quotes.
I'll review again once these issues are fixed
Hi @mademarc! Thanks for volunteering to review this issue! When you have a minute, please add your ETA and Availability. If for some reason you find that you're unable to review the request, please remove yourself as a reviewer. Thanks! |
Review ETA: 11/16/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 @djbradleyii good call on the changes for lines 58-63 and also on the adding of the double quotes either with or without linter.
298ee1b
to
1793487
Compare
@KyleA99 good catch. I fixed the issue. Let me know if you notice anything else. |
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.
@djbradleyii changes look great. Thank you for the great work!
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 @djbradleyii Great job on your first PR.
- Branches from/ to are good
- Linked back to original issue
- There is a concise description of what was done and why
- Relevant 'before' and 'after' screenshots are shown
- The code edits only change what needs to be changed
Everything looks good on your end- thanks!
You'll notice below that there is a merge conflict on this PR. If you have not seen this before, it just means that other people were working on the same file as you so the branch you started with is out of date. It is relatively easy to fix: First select "Resolve conflicts". The conflicts are shown on the next screen:
Your branch is showing 'Jodie Chen' and 'Yujin Chang' in Lucky Parking still, but if you look at #4909 these two have been removed by other issues. To resolve the conflict, you will want to delete these two profiles, so everything from lines 46-66. When done, select "Mark as resolved" and this should take care of the conflicts flag.
When you are done, select the chasing arrows next to my name to "re-request review".
Thanks for working on this issue!
@KyleA99 Now that the edits have been made, can you please approve the PR? Thanks. |
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 @djbradleyii - Awesome, thanks for fixing the merge conflict!
Fixes #5800
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