Refactor Tools data for tech-work-experience.md#5128
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. |
|
Review ETA: 8/2/23 EOD |
|
Availability 8/2 |
|
Hi @ziniwang, Issue is linked correctly, the naming convention of the branch is easy to follow, changes to the issue are documented and the code fix looks correct I can confirm that the site works and has the correct information on it when it is run in my local environment. Great work! |
wongstephen
left a comment
There was a problem hiding this comment.
Great job on your first issue pull request!
yujioshiro
left a comment
There was a problem hiding this comment.
Great job @ziniwang!
The branches are set up correctly, the changes reflect what the issue asks for, and the description is accurate.
While there are no visual changes to the site, because the issue asks that you confirm the tools still renders correctly, I think it would be good to upload images of the list being rendered correctly the next time.
I pulled @ziniwang's branch and confirmed that the list renders correctly both on the projects page as well as the Tech Work Experience page.
adrianang
left a comment
There was a problem hiding this comment.
Hi @ziniwang — the branching is set up correctly, the corresponding issue is linked, and the requested change of refactoring the Tools data for Tech Work Experience has been made exactly as requested in the linked issue. In testing your branch in my local environment through Docker, the change you made did not visually (except for title-casing 1Password, which seems to be more accurate anyway) or functionally affect the project in its various views (project pages and cards on mobile, tablet, and desktop).
Thank you for taking up this issue! 🙌🏼
Fixes #4913
What changes did you make?
tech-work-experience.mdfile from a string to a listWhy did you make the changes (we will use this info to test)?