-
-
Notifications
You must be signed in to change notification settings - Fork 799
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
added Leo Peng to file for issue#6104 #6148
added Leo Peng to file for issue#6104 #6148
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:
|
Availability: Tuesday 1/23 7pm-10pm, Wednesday 5pm-10pm 1/24, Friday 5pm-10pm 1/25 ETA: Friday 1/25 EOD |
ETA: 1/23/24 EOD |
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.
- Your PR is done with the correct branch.
- Your code needs to be modified slightly - see inline comment.
- These changes do actually affect the website visuals, please add a screenshot to your pull request once the code has been modified.
Feel free to reach out to me if you need any assistance.
Great work
Hi @kevin-421! Great job with the "what" and the "why" as well as the branching! In addition to any changes requested by your reviewers, please also remove the unused bullet points in the original pull request comment. Though it's non-functional, we like to keep them clean. I would also change @KyleA99 mentioned that the visuals do change. These changes will occur on the Communities of Practice page. The affected pages are often included in the resource section of the issue. If you have any questions, feel free to reach out. Thanks for taking the time to contribute and make updates! |
…update-communities-of-practice-ui-ux-add-leo-peng-6104
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.
PR approved. Great work @kevin-421
- Code is properly changed and looks great visually.
Thank you for your contributions!
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 @kevin-421
Like @KyleA99 said, Code and visuals look good.
Hey @kevin-421, thanks for making the changes requested by @KyleA99 and @LRenDO. I noticed the action items on the issue aren't checked off. Please make sure to mark them as completed. It helps track progress and keeps the team updated on what's done and what still needs attention. Also, an edit was made that's out of scope. It's minor, but to maintain consistency with other yml files, revert the removal of the blank line above |
…update-communities-of-practice-ui-ux-add-leo-peng-6104
Hello @jphamtv I see that you want me to check off actions items, but I cannot see where they are located can you point me in the right direction. And I also went ahead and deleted the blank line and pushed the code up thank you. |
Hey @kevin-421 , I'm sorry if my previous feedback wasn't clear. Let me try explaining again:
Let me know if this makes sense. I'm happy to clarify further if needed. Please re-request reviews once you've made the changes. To re-request reviews, click the icon with the arrows next to the reviewer names. Thanks! ![]() |
…update-communities-of-practice-ui-ux-add-leo-peng-6104
yes it makes sense @jphamtv thank you for clarifying. I went ahead and make the changes let me know If anything else needs correcting. Thank you! |
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 the consistent code markup as suggested from @jphamtv . I apologize I didn't account for that earlier.
The visuals support the change before and after. The branch commit matches.
Great Job @kevin-421
Hey @kevin-421, you're almost there. Just remove the indentation on line 46 and you should be all set. |
Hello @jphamtv correct me if I am wrong but I went ahead and looked at the rest of the code on the file. It looks like all of the indentation is the same for the "links:". |
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.
@kevin-421 I just looked over the current code for this PR and I believe that there is an indentation/nesting issue with "links:" on line 46. Please note that it appears we are using an internal "links:" for each leadership profile, and also a separate "links:" under/after the leadership profiles.
If you refer back to the original code (https://github.com/hackforla/website/blob/gh-pages/_data/internal/communities/ui-ux.yml), the "links:" is not indented/nested inside of the last leadership profile. I believe this issue is also throwing off the visuals, currently.
I'll be available all day if you have any questions. Feel free to message me on slack, or tag me in a comment here and we can converse.
Availability: Weekdays |
…update-communities-of-practice-ui-ux-add-leo-peng-6104
hello @KyleA99 I went ahead and removed the indentation, the visuals are now looking good. |
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.
Code and visuals appear properly changed now. Approved! Great job @kevin-421
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.
@kevin-421 , Thank you for working on this issue. Merge branches are correct. Code changes give desired results. Website works well in local machine. Well done. Approved!
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.
@KyleA99, thanks for providing detailed feedback on the indentation issue.
@kevin-421, appreciate your efforts on this pull request. All looks good now.
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.
branches are mergeable.
Code format is specified to standards.
Thank you!
Fixes #6104
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