-
-
Notifications
You must be signed in to change notification settings - Fork 374
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
Skript Enhancements #2519
Skript Enhancements #2519
Conversation
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.
Wow, incredible work, just a few things that might need to be fixed
aliases: | ||
# blocks without collision as of Minecraft 1.5 (excluding fire, nether portal & end portal) | ||
nonsolid = 0, 6, 27, 28, 30, 31, 32, 37, 38, 39, 40, 50, 55, 59, 63, 64, 65, 66, 68, 69, 70, 71, 72, 75, 76, 77, 78:0, 83, 85:4-7, 104, 105, 106, 111, 115, 131, 132, 141, 142, 143, 147, 148, 157 | ||
|
||
on rightclick with compass: | ||
player has permission "skript.teleport" | ||
loop blocks above targeted block: |
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.
I'd rather use a simple if check rather than the where filter, it'd confuse new people.
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.
Yeah so the condition has to be brought back in
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.
But how nonsolid
blocks are usefull here? I mean, I did tests without this condition and it works really well.
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.
Does it not prevent the player from being teleported inside a block ?
Also I think you should format the condition with if
like you did in other examples
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.
When you use if
word, you are supposed to follow with the else
part, or you just use it as "code block" so here you are not supposed to use the if
word because it's an "affirmative" condition (so if this condition is not respected, it will not continue the code)
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.
Yeah that's what I do but I would write it there as a convention, in Java or Python you cannot omit it
aliases: | ||
# blocks without collision as of Minecraft 1.5 (excluding fire, nether portal & end portal) | ||
nonsolid = 0, 6, 27, 28, 30, 31, 32, 37, 38, 39, 40, 50, 55, 59, 63, 64, 65, 66, 68, 69, 70, 71, 72, 75, 76, 77, 78:0, 83, 85:4-7, 104, 105, 106, 111, 115, 131, 132, 141, 142, 143, 147, 148, 157 | ||
|
||
on rightclick with compass: | ||
player has permission "skript.teleport" | ||
loop blocks above targeted block: |
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.
I'd rather use a simple if check rather than the where filter, it'd confuse new people.
Should probably configure your git to not treat every file as being completely different so we can actually tell what was modified :P |
The files changed section is displayed by GitHub itself |
Didn't know that was possible but it still seems weird 🤔 |
Co-Authored-By: Giovanni <42092549+Matocolotoe@users.noreply.github.com>
Co-Authored-By: Giovanni <42092549+Matocolotoe@users.noreply.github.com>
Co-Authored-By: Giovanni <42092549+Matocolotoe@users.noreply.github.com>
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.
Note: I didn't review the example script changes yet. This review is only for documentation site changes.
You can also include other files by using ${include <filename>}. Just please make | ||
sure that those included files don't have tags which are not allowed in position | ||
where include is called. | ||
**Using NPM:** |
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.
People exporting documentation should not be required to install anything in addition to Minecraft server + Java.
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.
I'll clarify this a bit: currently, on Linux, Windows and Mac, Skript needs JDK 8 or newer to be compiled. Gradle wrapper provides Gradle, which in turn provides all compile-time dependencies. So, only JDK needed to produce a usable jar.
Spigot installation is, at the moment, needed for generating documentation. We could probably get rid of this, though, and just prepare classpath with dependencies (Guava, etc.) in build script.
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.
Documentation should be not related to any minecraft server
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.
the doc site is generated within the plugin itself, so it runs where the plugin runs (a minecraft server)
which provides head element, navigation bar and so on. | ||
* Bulma | ||
* FontAwesome | ||
* Svelte |
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.
Documentation website must work without Javascript, or at very least there must be alternative that works.
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.
Modern websites use javascript. What's the problem with javascript?
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.
It is not necessary for static pages. For dynamic features such as search, there is no alternative.
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.
Agree with you. But modern technologies (as react, and here svelte) use javascript to create modules. These modules are render on the website directly. So without javascript, Svelte, react and more component technologies will not work.
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.
At least React supports rendering static content on server. Most frameworks do, though I don't know about Svelte.
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.
Svelte too. Svelte is compiled using a bundler (as webpack or my favorite, rollup) into a static js file, what the website is using.
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.
in this context, static doesn't mean that the files are the same every time - it means that the site has no dynamic content (like DOM manipulation and such via JS). The reason we don't want JS is because it just isn't necessary - it increases page load and parse times for no real benefit for a site as simple as Skript's docs.
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.
I'm agree with you, but we could add more features later, that's the reason of my choice.
I forgot to say that the size doesn't matter. Sapper uses service workers and cache any content. So the javascript is cached into the client's browser. It's really faster. Plus, svelte is probably the smaller javascript librairy existing (comparing to react, vue etc...)
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.
Web browsers are pretty good at caching static pages too. What I'd like to see is static pages with dynamic features implemented in JS. If that is not feasible, static pages when JS is not enabled, otherwise everything done with JS. If even that can't be done, two sites for browsing docs wouldn't be any worse than current situation. In fact, I suggested the last option when you were initially working on this PR.
# A join and leave message. | ||
# | ||
|
||
command /set <string> message <text>: |
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.
command /set <string> message <text>: | |
command /set <text> message <text>: |
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.
Should be better to use string instead of text type right?
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.
Skript handles both but at least if you want to use one in particular, use it everywhere
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.
Yeah I missed to change the 2nd text with string
. I will fix that.
else: | ||
message "Only 'join' and 'leave' messages are available here." | ||
|
||
command /show <string> message: |
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.
command /show <string> message: | |
command /show <text> message: |
"Skript maintainer", | ||
"Skript Developer" | ||
] | ||
}, | ||
"FranKusmiruk": { | ||
"avatar": "https://avatars3.githubusercontent.com/u/48283695?s=400&v=4", | ||
"roles": [ | ||
"Skript Developer" | ||
] | ||
}, | ||
"Pikachu920": { | ||
"avatar": "https://avatars0.githubusercontent.com/u/28607612?s=400&v=4", | ||
"roles": [ | ||
"Skript Developer" | ||
] | ||
}, | ||
"Nicofisi": { | ||
"avatar": "https://avatars2.githubusercontent.com/u/12614053?s=400&v=4", | ||
"roles": [ | ||
"Skript Developer" | ||
] | ||
}, | ||
"TheBentoBox": { | ||
"avatar": "https://avatars3.githubusercontent.com/u/6902229?s=400&v=4", | ||
"roles": [ | ||
"Issue tracker manager", | ||
"Aliases developer" |
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.
Consistency seems a little off in terms of casing.
You have Skript Developer
and then Skript maintainer
I recommend sticking with upper casing for each word, but thats just my opinion.
If not, then lower case developer to match the rest.
"description": "What is a loop? And how make loops in Skript? Look here!" | ||
}, | ||
"Functions": { | ||
"description": "The most usefull thing in any language: functions!" |
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.
"description": "The most usefull thing in any language: functions!" | |
"description": "The most useful thing in any language: functions!" |
"description": "The most usefull thing in any language: functions!" | ||
}, | ||
"Custom Commands": { | ||
"description": "The most usefull thing in Skript: custom commands!" |
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.
"description": "The most usefull thing in Skript: custom commands!" | |
"description": "The most useful thing in Skript: custom commands!" |
I close this pull request to split it better and make it more easily to review. Thanks to everyone who reviewed this pull request. |
Description
All referenced issues in #2506 have been fixed in this pull request.
Target Minecraft Versions: none
Requirements: none
Related Issues: #2506