Skip to content
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

Addresses review comments on Issue 105 #106

Merged
merged 3 commits into from
Jan 20, 2025

Conversation

astroDimitrios
Copy link
Member

@astroDimitrios astroDimitrios commented Jan 14, 2025

Fixes #105
Fixes #104

@astroDimitrios
Copy link
Member Author

image

Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @astroDimitrios! 🥳

We discussed:

  • moving the first paragraph to the next section (I find that opening chunk of text a little overwhelming)
  • adding a "Prerequisites" heading so that it shows in the left hand panel

but that first paragraph is in a special file that doesn't recognise any headings, so we agreed to leave it as it is, but I just had a thought 🤪 Would it be possible to add an opening sentence like: "Welcome to the Introduction to Git and GitHub training! This page contains learning outcomes and setup that must be completed before participating in the training" or equiavalent? Then go into the version control summary?

Would it be possible to move the Version control link to the Version Control System text in the next sentence, please?

Should the "Your instructor and organisation" sentence in the "Passkeys" section also be removed? 🤔

We also just discussed:

index.md Outdated Show resolved Hide resolved
learners/setup.md Outdated Show resolved Hide resolved
learners/setup.md Outdated Show resolved Hide resolved
learners/setup.md Outdated Show resolved Hide resolved
learners/setup.md Outdated Show resolved Hide resolved
learners/setup.md Show resolved Hide resolved
learners/setup.md Show resolved Hide resolved
learners/setup.md Outdated Show resolved Hide resolved
learners/setup.md Show resolved Hide resolved
@astroDimitrios astroDimitrios requested a review from ehogan January 20, 2025 10:27
@ehogan
Copy link
Member

ehogan commented Jan 20, 2025

Thanks @astroDimitrios! 🎉 I believe the outstanding issues are:

  1. Is there an issue (in Software Carpentry?) somewhere to track the outcome of the discussion we had related to:
  • moving the first paragraph to the next section (I find that opening chunk of text a little overwhelming)
  • adding a "Prerequisites" heading so that it shows in the left hand panel

but that first paragraph is in a special file that doesn't recognise any headings, so we agreed to leave it as it is.

  1. This comment wasn't addressed:

Should the "Your instructor and organisation" sentence in the "Passkeys" section also be removed? 🤔

  1. I still haven done:
  1. Is there an issue somewhere to track:

Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One very minor additional suggestion 😊

learners/setup.md Show resolved Hide resolved
Copy link
Member

@ehogan ehogan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@astroDimitrios would prefer the comment at point 2 above remains, so the only outstanding issues are opening new issues. Approving 🥳

@astroDimitrios
Copy link
Member Author

An Issue has been created on the original SWC lesson to discuss moving the Summary and Setup intro text to the first episode:
swcarpentry#1062

@ehogan
Copy link
Member

ehogan commented Jan 20, 2025

  1. I still haven done:

I added a comment to https://github.com/MetOffice/azure-spice-docs/issues/194#issuecomment-2602427619 😊

@astroDimitrios astroDimitrios merged commit 909f4e9 into MetOffice:main Jan 20, 2025
1 check passed
@astroDimitrios astroDimitrios deleted the 105_summary_setup branch January 20, 2025 13:30
github-actions bot pushed a commit to astroDimitrios/git-novice that referenced this pull request Jan 20, 2025
Auto-generated via `{sandpaper}`
Source  : 909f4e9
Branch  : main
Author  : Dimitrios Theodorakis <[email protected]>
Time    : 2025-01-20 13:30:27 +0000
Message : MetOffice#106 Addresses review comments on Issue MetOffice#105 Summary and Setup feedback

* Addresses review comments on Issue 105

* Address review comments

* Add version number disclaimer and note about nano and cat command usage
github-actions bot pushed a commit to astroDimitrios/git-novice that referenced this pull request Jan 20, 2025
Auto-generated via `{sandpaper}`
Source  : 858da6b
Branch  : md-outputs
Author  : GitHub Actions <[email protected]>
Time    : 2025-01-20 13:32:13 +0000
Message : markdown source builds

Auto-generated via `{sandpaper}`
Source  : 909f4e9
Branch  : main
Author  : Dimitrios Theodorakis <[email protected]>
Time    : 2025-01-20 13:30:27 +0000
Message : MetOffice#106 Addresses review comments on Issue MetOffice#105 Summary and Setup feedback

* Addresses review comments on Issue 105

* Address review comments

* Add version number disclaimer and note about nano and cat command usage
github-actions bot pushed a commit to astroDimitrios/git-novice that referenced this pull request Jan 21, 2025
Auto-generated via `{sandpaper}`
Source  : 858da6b
Branch  : md-outputs
Author  : GitHub Actions <[email protected]>
Time    : 2025-01-20 13:32:13 +0000
Message : markdown source builds

Auto-generated via `{sandpaper}`
Source  : 909f4e9
Branch  : main
Author  : Dimitrios Theodorakis <[email protected]>
Time    : 2025-01-20 13:30:27 +0000
Message : MetOffice#106 Addresses review comments on Issue MetOffice#105 Summary and Setup feedback

* Addresses review comments on Issue 105

* Address review comments

* Add version number disclaimer and note about nano and cat command usage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants