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

Add Getting started tutorial and link it between pages #64

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

milanjaros
Copy link

@milanjaros milanjaros commented Dec 25, 2024

Related to #12:

  • Add Getting Started tutorial.
  • Add a link to the navbar.
  • Add a link to the Index page.
  • Add a link to the Text page.
  • Add a link to the Tutorials page.
  • Fix a command to generate docs from a template in the README.md.

@milanjaros
Copy link
Author

@sovdeeth please, could you consider approval of these changes too?

@sovdeeth
Copy link
Member

@sovdeeth please, could you consider approval of these changes too?

please don't tag people for reviews, we all get notifications about new prs and will review when we have time.

@milanjaros
Copy link
Author

milanjaros commented Dec 29, 2024

Sorry, sure, I was under impression that this docs project is forgotten… Since there are 7 open Pull Requests. I know you do it in your free time, in good will. 🙏

@milanjaros
Copy link
Author

Is there anything I can do for you to simplify review? Should I add screenshots?

Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution ⚡

Just few notes :)

3. The `docs/` directory will be created _(if not created already)_ in `plugins/Skript` containing the website's files.
4. Open `index.html` and browse the documentation.
5. _(Optionally)_ Add this system property `-Dskript.forceregisterhooks` in your server startup script (before the -jar property) to force generating hooks docs.
2. Launch the server (the [Paper](https://papermc.io/), which is a fork of Spigot, is recommended).
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should specify such things here in case it gets changed later.

Suggested change
2. Launch the server (the [Paper](https://papermc.io/), which is a fork of Spigot, is recommended).
2. Once created, launch the Minecraft server

Copy link
Author

Choose a reason for hiding this comment

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

If I may, I would disagree. I found the information in the readme. I'm new to MC development, and I Installed a different and incompatible server because I was missing this information.

README.md Outdated Show resolved Hide resolved
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali Jan 7, 2025

Choose a reason for hiding this comment

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

Nice work! just couple notes:

  • Use "Minecraft server" instead of "Paper server"
  • Prefer using single paragraphs with multiline text than multiple single paragraphs
  <p class="subtitle">
    Skript requires Spigot to work. You heard it right, CraftBukkit does not work. <a
      href="https://papermc.io/">Paper</a>, which is a fork of Spigot, is recommended; it is required for some parts of
    Skript to be available.
-  </p>
-  <p class="subtitle">
    Skript supports only the latest patch versions of Minecraft. For example, this means that if 1.16.5 is supported,
    then 1.16.4 is not. Testing with all old patch versions is not feasible for us.
  </p>

  <p class="subtitle">
    Skript requires Spigot to work. You heard it right, CraftBukkit does not work. <a
      href="https://papermc.io/">Paper</a>, which is a fork of Spigot, is recommended; it is required for some parts of
    Skript to be available.
+   <br>
    Skript supports only the latest patch versions of Minecraft. For example, this means that if 1.16.5 is supported,
    then 1.16.4 is not. Testing with all old patch versions is not feasible for us.
  </p>

Copy link
Author

Choose a reason for hiding this comment

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

Done. I replaced "Paper server", but two occurrences where I would like to leave the "Paper":

Copy link
Author

Choose a reason for hiding this comment

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

For me, it was a pain to figure out that I needed to use the "Spigot compatible" server. If you are not against, I would keep this information wherever it is possible.

docs/templates/getting-started.html Outdated Show resolved Hide resolved
docs/templates/getting-started.html Outdated Show resolved Hide resolved
docs/templates/getting-started.html Outdated Show resolved Hide resolved
Comment on lines 62 to 71
<div id="info" class="grid-container padding">
<div class="grid-item">
</div>
<div class="grid-item">
</div>
<div class="grid-item">
<p class="box-title">Next step</p>
<p class="box placeholder"><a class="link" href="getting-started.html">Getting started</a></p>
</div>
</div>
Copy link
Member

Choose a reason for hiding this comment

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

Let's make this take the full width instead

Suggested change
<div id="info" class="grid-container padding">
<div class="grid-item">
</div>
<div class="grid-item">
</div>
<div class="grid-item">
<p class="box-title">Next step</p>
<p class="box placeholder"><a class="link" href="getting-started.html">Getting started</a></p>
</div>
</div>
<p class="box-title">Next step</p>
<p class="box placeholder">Visit the <a class="link" href="getting-started.html">Getting started</a> page to learn more on how to install Skript and create your first script!</p>

Copy link
Author

Choose a reason for hiding this comment

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

Okay, let me check this first. I'll compare the results.

Copy link
Author

Choose a reason for hiding this comment

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

Looks better. Done.

docs/templates/text.html Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Let's remove these changes as we have other plans for a new design for this page :)

Copy link
Author

Choose a reason for hiding this comment

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

Removed.

@AyhamAl-Ali AyhamAl-Ali added enhancement Feature request, an issue about something that could be improved, or a PR improving something. tutorials A PR/Issue related to tutorials labels Jan 7, 2025
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/templates/getting-started.html Outdated Show resolved Hide resolved
docs/templates/getting-started.html Outdated Show resolved Hide resolved
docs/templates/getting-started.html Outdated Show resolved Hide resolved
docs/templates/getting-started.html Outdated Show resolved Hide resolved
milanjaros and others added 5 commits January 7, 2025 22:05
Co-authored-by: Ayham Al Ali <20037329+AyhamAl-Ali@users.noreply.github.com>
Co-authored-by: Efnilite <35348263+Efnilite@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something. tutorials A PR/Issue related to tutorials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants