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

Getting Started Guide #831

Merged
merged 27 commits into from Feb 2, 2023
Merged

Getting Started Guide #831

merged 27 commits into from Feb 2, 2023

Conversation

simplePCBuilding
Copy link
Contributor

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.

@simplePCBuilding
Copy link
Contributor Author

I will fix the issues rn

Copy link
Contributor

@henrikingo henrikingo left a 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!

src/plugins/impressConsole/impressConsole.js Outdated Show resolved Hide resolved
@simplePCBuilding
Copy link
Contributor Author

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

@simplePCBuilding
Copy link
Contributor Author

I just got it... it works by adding additional branches right?

Copy link
Contributor

@henrikingo henrikingo 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!

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.

GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Show resolved Hide resolved
@simplePCBuilding simplePCBuilding changed the title Getting Started Guide and improved notes hiding Getting Started Guide Dec 13, 2022
@simplePCBuilding
Copy link
Contributor Author

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.

@simplePCBuilding
Copy link
Contributor Author

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.

@simplePCBuilding
Copy link
Contributor Author

https://cdn.jsdelivr.net/npm/impress.js@1.1.0/js/impress.js

There we go. jsdelivr is the solution

@simplePCBuilding
Copy link
Contributor Author

What happened?

@simplePCBuilding
Copy link
Contributor Author

simplePCBuilding commented Dec 14, 2022

Wait... is this due to the links that it failed?

Copy link
Contributor

@henrikingo henrikingo left a 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.

GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
@simplePCBuilding
Copy link
Contributor Author

I just changed the wording regarding jsDelivr to better explain the usage.

@simplePCBuilding
Copy link
Contributor Author

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)

@henrikingo
Copy link
Contributor

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.

@simplePCBuilding
Copy link
Contributor Author

Okay. I think I cannot add anything to the releases, right? Also, what do you think about the latest changes?

@simplePCBuilding
Copy link
Contributor Author

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.

Copy link
Contributor

@henrikingo henrikingo left a 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

GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
GettingStarted.md Outdated Show resolved Hide resolved
GettingStarted.md Show resolved Hide resolved
@henrikingo
Copy link
Contributor

Okay. I think I cannot add anything to the releases, right?

Ah right. Can you file a new ticket and me or someone else could look into it in the future.

@simplePCBuilding
Copy link
Contributor Author

I have updated my repo to include the changes you mentioned

Copy link
Contributor

@henrikingo henrikingo left a 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.

GettingStarted.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@simplePCBuilding
Copy link
Contributor Author

I saw it, thank you

@simplePCBuilding
Copy link
Contributor Author

I just checked again that I didn't miss any of the previously mentioned things, I didn't

GettingStarted.md Outdated Show resolved Hide resolved
@simplePCBuilding
Copy link
Contributor Author

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.

@simplePCBuilding
Copy link
Contributor Author

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?

Copy link
Contributor

@henrikingo henrikingo left a 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.

GettingStarted.md Show resolved Hide resolved
GettingStarted.md Show resolved Hide resolved
src/impress.js Show resolved Hide resolved
src/impress.js Outdated Show resolved Hide resolved
@simplePCBuilding
Copy link
Contributor Author

fixed everything you mentioned. Also I think it is important that this has a high standard

@henrikingo
Copy link
Contributor

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!

@henrikingo henrikingo merged commit 4edb9e0 into impress:master Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants