-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Getting Started Guide #831
Conversation
I will fix the issues rn |
Fixed an error and added more infos
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 thanks this is amazing! I will have to read it with thought tomorrow. Thanks for writing this!
This reverts commit 40a3766.
I have not really used PRs before, so I'm not sure how to have multiple PRs from the same repo (so my fork where I pushed the changes). It appears that whenever I push to that repo, the PR updates... I will look up how to do it today and will update the PR asap |
I just got it... it works by adding additional branches 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.
Thank you!
I made a few comments which you may or may not want to incorporate. Since this is not a code review, I'm fine leaving the final editorial judgement to you, but wanted to add some additional viewpoints where I had any.
I've just come up with a great idea to make the usage easier. Afaik, you can include a js file from raw.githubusercontent.com... If this is so, one can just copy that link with a script tag into the head of the html file and therefore you don't have to download it. |
Doesn't appear to work. I might be able to pull something off that is slightly different. I have only tested it on my iPad so this might be the problem as well. |
https://cdn.jsdelivr.net/npm/impress.js@1.1.0/js/impress.js There we go. jsdelivr is the solution |
What happened? |
Wait... is this due to the links that it failed? |
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.
Ok so looks good otherwise, but now we can fine tune the jsdelivr version of impress.js. This conversation can also continue in the separate ticket.
I just changed the wording regarding jsDelivr to better explain the usage. |
I feel like we should also start to provide .min.js files for the cdn. I could update the package.json to include minify (or browserify for that matter) |
Note that build.js already creates a minified file. It's just not committed to the git repo. But adding it to release pages and CDN is a great idea. |
Okay. I think I cannot add anything to the releases, right? Also, what do you think about the latest changes? |
LOL that was weird. I commented on my phone which at school has really bad connection, GitHub spat out a network error, but my comment got actually published. |
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.
Some more comments on the text
Ah right. Can you file a new ticket and me or someone else could look into it in the future. |
I have updated my repo to include the changes you mentioned |
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.
Thanks. You missed a couple and I added 2 more comments.
I saw it, thank you |
I just checked again that I didn't miss any of the previously mentioned things, I didn't |
I have also bumped the version number to 2.1.0 in the impress.js file. It was still on V1.1.0. I might be wrong with that version number, but it'd make sense, as 2.0.0 is already out. |
Also, I just noticed, that the copyright info is really outdated in the README.md. Shall I update it, or will somebody else do it? |
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.
Thanks. This is really good now.
Sorry, I'm going back and forth I guess, but I have to ask you to change the part about CSS some more. Other than that I'm quite satisfied.
fixed everything you mentioned. Also I think it is important that this has a high standard |
Yes, I appreciate your dedication and patience. It's not so much that I hesitate to strive for perfection... I just feel bad if you've waited for review 4-6 weeks and then I ask to change one more thing. But now this is done! Thanks so much for doing this! |
For me, getting started with impress.js was not that easy, honestly, in retrospective, I don't know why. So I created a little "Getting Started" guide to help new users starting out. I also noticed a problem, where the notes didn't hide as they should have. So I added some code that takes care of that (it just sets the CSS display property to "none" on all objects with class "notes". Tested it, it worked.)
I will also be creating an introduction video in a short while and link it in the Getting Started Guide.