-
-
Notifications
You must be signed in to change notification settings - Fork 798
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 Docker info in CONTRIBUTING.md Section 2.7.b #7631
Update Docker info in CONTRIBUTING.md Section 2.7.b #7631
Conversation
Want to review this pull request? Take a look at this documentation for a step by step guide! 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.
@jenjenkayi Thank you for taking on this issue.
I'm not sure why I am not able to see the changes through the link that you provide in the pr.
It seems like you made the correct changes. Are you able to see the changes on your end from the url that you provided ?
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, everything looks solid 👍 and the link you provided did work for me.
The one thing I noticed is that the hyperlinks at the top of the CONTRIBUTING.md
page no longer work for sections 2.7b
and i
due to the title name changes.
My suggestion is to:
-
edit the hyperlinks at the top of the
CONTRIBUTING.md
file so that they reference the new title names for2.7b
and its subsectioni
so they can continue to be easily accessible via the table of contents hyperlinks. -
after completing this change double-check your work to ensure that clicking each of those hyperlinks redirect you to sections
2.7b
andi
, respectively.
If you have any questions don't hesitate to reach out.
Thanks for making the adjustments, @jenjenkayi! I'll let someone who has more authority than me take a look at these notes and make the final decision on subsection
|
@pluto-bell Thank you for reviewing! I have checked both links, they are working now. |
@codyyjxn Thank you for reviewing! Yes, the changes are up to date from the url provided. |
Review ETA: 2 PM 10/26/24 |
-Change "the Docker Desktop application..." with "The Docker Desktop application..." in the first bullet point. -I would personally keep the last two bullet points from the previous version and add them at the end.
-In the new Docker version when clicking the three dots, I don't see the option "View details" instead I see the option "View packages and CVEs" I would add that just in case. |
@santisecco Updated per your comments. Please review. 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.
Hi @jenjenkayi thank you for the updates and adding the screenshot for the new version of docker. It looks great.
Thanks for working on this.
@pluto-bell I have updated the changes. Please review it again. Thank you |
@jenjenkayi Thank you for making the changes. Please request a review so i can approve this. |
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.
@jenjenkayi Thank you for taking this issue on. Keep it up !
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 @jenjenkayi Thank you for working on this! Everything is looking great and it appears that you have satisfied the original issue. There are a couple of edits, however, that I believe we will want to make:
- As @pluto-bell had mentioned earlier, I agree that the header for 2.7.i should be a title, and currently this reads as a paragraph. I think that the best solution is to change the text on line 55 to something like (and please check that this is formatted correctly):
and line 249 to (please remove the space between the link and the period):
- [**i. A few notes regarding Docker:**](#i-a-few-notes-regarding-docker)
##### **i. A few notes regarding Docker:** - To test your issue branch locally, run the command "docker-compose up" from a terminal window. The website will then be accessible at http://localhost:4000.
- Also, on line 552 there is a misspelling "...which enables Docker to built the website locally..." Here "built" should be "build"
Again, you followed the issue correctly but would you please make these edits?
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 @jenjenkayi This looks great! Thanks for making the edits.
Fixes #7576
What changes did you make?
Why did you make the changes (we will use this info to test)?
For Reviewers: Do not review changes locally, rather, review changes at https://github.com/jenjenkayi/website/blob/update-contributing.md-7576/CONTRIBUTING.md.
Screenshots of Proposed Changes To The Website (if any, please do not include screenshots of code changes)