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.

Sign up for GitHub

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

Jump to bottom

docs: Dev vs Master mixups #358

Merged
merged 9 commits into from
Mar 11, 2021
Merged

docs: Dev vs Master mixups #358

merged 9 commits into from
Mar 11, 2021

Conversation

weichweich
Copy link
Contributor

@weichweich weichweich commented Mar 11, 2021 •
edited
Loading

Removes Links to Develop from Master branch

Some links are hard coded to point to the master or develop branch. This can be very confusing.

Looks like GitHub can resolve paths in the local repository. Removing the domain and branch info prevents jumps between develop and master branch.

We might need to check how this works, when the README is displayed in the API doc or on NPM.

How to test

  • ho to the start page
  • select this branch
  • click on links to other documentation

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@weichweich
remove absolute link path
cbd04f8 
looks like github can resolve paths in the local repository.
Removing the domain and branch info prevents jumps between develop and master branch
@weichweich
add develop vs master branch warning
9f21b8f
@weichweich weichweich requested a review from ntn-x2 March 11, 2021 08:43
@weichweich weichweich changed the title Doc: Remove absolute link path Doc: Dev vs Master mixups Mar 11, 2021
@weichweich
Copy link
Contributor Author

weichweich commented Mar 11, 2021 •
edited
Loading

@Diiaablo95 I'm not sure about the warning. It's now right above the section where all the links point to the Master branch. 😂 so i removed it again

@weichweich
Copy link
Contributor Author

I added a warning that doesn't need to be removed when this will merge into master. But I'm still not really satisfied.

ntn-x2
ntn-x2 requested changes Mar 11, 2021
View reviewed changes
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

I would just change a little bit the working of the warning, and display it with the > tag, which is usually used for warnings.

packages/sdk-js/README.md Outdated
- [Getting started guide](https://github.com/KILTprotocol/sdk-js/blob/master/docs/getting-started.md) 👈 Start here if you'd like to include KILT in your project
- [KILT Developer overview](https://dev.kilt.io/) 👈 Checkout for an overview of the codebase, infrastructure and deployed KILT instances.
- [API documentation](https://kiltprotocol.github.io/sdk-js)
- [KILT workshop (Release Version)](https://github.com/KILTprotocol/kilt-workshop-101) 👈 Start here to get familiar with the basics
Copy link
Member

Choose a reason for hiding this comment

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

Should this also be releaseD?

packages/sdk-js/README.md Outdated
@@ -18,6 +18,8 @@ Regarding the privacy enhancement, please have a look at our [lightning talk for

## Documentation

* The Links found here are related to the `master` branch aka the current released version. * Links outside this section point to the version you are currently viewing.
Copy link
Member

Choose a reason for hiding this comment

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

I would rather use the > tag and optionally make the sentence bold, even thought that might be too much.

Copy link
Contributor Author

@weichweich weichweich Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ah didn't had a look at the preview. It should have been bold. :D That's why there is a * in the middle of the sentence. 😁

packages/sdk-js/README.md Outdated
@@ -18,6 +18,8 @@ Regarding the privacy enhancement, please have a look at our [lightning talk for

## Documentation

* The Links found here are related to the `master` branch aka the current released version. * Links outside this section point to the version you are currently viewing.
Copy link
Member

Choose a reason for hiding this comment

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

current -> latest?

Copy link
Member

Choose a reason for hiding this comment

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

I would write something like "to avoid confusion between the latest released SDK version and the develop default branch, the links in the list that follow point to the master branch, which always contains the latest official release of the SDK.

ntn-x2
ntn-x2 reviewed Mar 11, 2021
View reviewed changes
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

I added one more suggestion. It is personal, so I won't mark it as a required change.

packages/sdk-js/README.md
@@ -18,6 +18,8 @@ Regarding the privacy enhancement, please have a look at our [lightning talk for

## Documentation

> To avoid confusion between the latest released SDK version and the `develop` default branch, the links in the list **point to the master branch**, which always contains the latest official release of the SDK.
Copy link
Member

Choose a reason for hiding this comment

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

  • list below or following list
  • I would also make bold the whole sentence "the links in the list point to the master branch"

@weichweich weichweich changed the title Doc: Dev vs Master mixups docs: Dev vs Master mixups Mar 11, 2021
@weichweich weichweich merged commit 9ed2db6 into develop Mar 11, 2021
@weichweich weichweich deleted the aw-develop-master-confusion branch March 11, 2021 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntn-x2 ntn-x2 ntn-x2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@weichweich @ntn-x2