-
-
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
changed tools items format #5123
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.
|
Availability 8/2 12pm - 5pm |
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 @Vinny02
Great job on the first issue. You documented the changes correctly.
For the pull request comments, I recommend paraphrasing the description from the issue for clarity. Such as 'The filter menu on the projects-check page requires modification to incorporate a "Tools" dropdown.'
In addition, I do not believe it is necessary to add a screenshot as there are no visual changes to the website.
I can confirm the changes are correct and visually accurate when ran in my local environment. The card is hidden (maybe because the project is marked complete) but can be found at https://www.hackforla.org/projects/civic-tech-structure.
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 @Vinny02 — going through PRs and I noticed a couple things that should be addressed and edited before this gets merged and closed:
- The 'Why' section of your PR should be a bit more descriptive, more within the context of the issue itself. The Overview in the linked issue contextualizes the reason for this change, so I would edit this section with a quick blurb about why this needs to be in a list format. @wongstephen also provided a great example above!
- I also agree with @wongstephen that this issue doesn't quite need a screenshot, but if you do provide a screenshot (for this issue and future ones), please screenshot the corresponding page on the website and not the Markdown file; the screenshot of what the user sees would be more useful to document.
Everything else is looking good so far (branching, linked issue, change itself) — just the documentation on the PR itself needs changes.
Fixed! I used your suggestion for this one @wongstephen, and I'll follow your recommended strategy to paraphrase the issue in my future PRs. Thank you for the assistance as well @adrianang. |
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 @Vinny02 — the PR looks good to me! The branching is set up correctly, the corresponding issue is linked, and the requested change was made correctly. Your branch is also showing up on my local environment through Docker and the Civic Tech Structure project visually and functionally remains unchanged.
Thank you for editing the message in your PR and for taking up this issue! 🙌🏼
Fixes #4801
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)
No changes to site