-
-
Notifications
You must be signed in to change notification settings - Fork 778
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
Updated tools section of metro-ontime.md by removing Docker, and changed format from string to list. #5183
Updated tools section of metro-ontime.md by removing Docker, and changed format from string to list. #5183
Conversation
…ging format from string to list.
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.
|
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 @joelfuelling!
Thanks for sticking with it to get the formatting and missing directory sorted out!
You did a great job with
- Branching correctly
- Linking the issue
- Making the correct changes
- Looping the original reviewers into the new PR
- Showing the closed PRs
I have a few non-functional requests
- The
![image]
before the images and the)
after images can be deleted. This is unnecessary because when you drag and drop or copy into the comment field GitHub automatically format the markdown text with the file name as the alt text. It's also good idea to make sure the alt description between the brackets to be descriptive of the image itself. - For the why changes were made section, I would also add something that includes the reasoning for changing the tools from a string to a list which is included in the issue's Overview section.
- In the future, it's a good idea to separate out the PR info related to the fix for the issue from any explanation about the PR process itself. In other words, only the information to fill out the PR template would be in the initial opening PR comment. Other comments about the other PRs can be placed in a separate comment after the PR is created, so that it is easy and clear for the reviewers to understand the issue and it's solution.
Thanks again for your time, persistence, and contribution!
ETA: 8/12/23 |
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.
@joelfuelling Good job on addressing the requested changes in the projects/metro-ontime.md
file. For future pull request's, I advise taking the time to click the preview tab at the top, to the right of the write tab, to see what the submitted pull request will look like. That way, if there are any tags that are not closed/utilized, you will see it right away. This will help with readability.
Hi, Travis. That is a wonderful piece of advice, thank you! |
Fixes #4810
Reviewers: I'm assigning all 3 of you since you've been helping with this since my initial push attempt.
After talking to Ren (thanks again!) I just reforked and following the correct procedure, this is the corrected push with ONLY the changes to the metro-ontime.md file per the instructions. My VScode no longer auto formats, or alters .vscode/settings.json files like it did with my initial attempt. I've included a screen shot at the bottom of this comment of the closed pull attempts from the project board.
What changes did you make?
"tools: Docker, AWS, Observable", to...
"tools:
Why did you make the changes (we will use this info to test)?
Screenshots of Proposed Changes Of The Website
Visuals before changes are applied
Visuals after changes are applied
Screenshot of closed branches from project pages.