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

API reference docs #563

Closed
wants to merge 19 commits into from
Closed

API reference docs #563

wants to merge 19 commits into from

Conversation

FagnerMartinsBrack
Copy link
Member

@FagnerMartinsBrack FagnerMartinsBrack commented May 21, 2016

This starts the reference docs for impress.js as stated in the plan.

@bartaz, this is important because it represents the public contract for consumers of the framework. We should be safe to change anything that is not publicly documented. See here for a detailed information of why it is very important to have a public API that doesn't rely solely in code comments.

@FagnerMartinsBrack FagnerMartinsBrack added this to the v0.6.0 milestone May 21, 2016
@FagnerMartinsBrack FagnerMartinsBrack changed the title Work in Progress: API reference docs API reference docs May 22, 2016
@FagnerMartinsBrack
Copy link
Member Author

@impress/mergers Please review.

@Pierstoval
Copy link
Contributor

I don't know if this comment has its place here or in another issue but...

What's the difference between .present and .active exactly?

@FagnerMartinsBrack
Copy link
Member Author

FagnerMartinsBrack commented May 23, 2016

I don't know if this comment has its place here or in another issue but...
What's the difference between .present and .active exactly?

That's actually a very good question!

Looking at the code, the .present class is added and removed when the events impress:stepenter and impress:stepleave are executed.

The impress:stepenter is executed whenever the step is selected via goto, either manually or programatically. The impress:stepleave is the same, except that it is executed when leaving the step.

The .active class seems to be added and removed before any of those events are executed. Not sure if there is a practical reason for doing so, and I haven't actually tested it in jsbin or something like that.

So, from all I can tell, the .active class and any animation based on it will start executing before the .present class is added.

In the blame, I can see that the original commit that added the .present feature was this one: b23506e. The version 0.5 changelog doesn't say much.

The API was added in the version 0.3.0 in this commit, which represents the first version of impress.js.

Conclusion

It doesn't seems to be any documented reason of why this both classes exist, all evidence points out to be a coincidence.

Note: That is the reason why it is important to have clear, consistent and legible commits

@Pierstoval
Copy link
Contributor

In the docs, I think there should be a small explanation of the list of all events/things that happen when slides change (when clicking on a step element, or using .goto() or .next(), etc.), to be more clear about what happens

@FagnerMartinsBrack
Copy link
Member Author

FagnerMartinsBrack commented May 23, 2016

@Pierstoval Totally agree.

  • Add documentation for the impress:stepenter and impress:stepleave events.

@FagnerMartinsBrack
Copy link
Member Author

FagnerMartinsBrack commented May 28, 2016

  • Add documentation for the impress:init event.

@FagnerMartinsBrack
Copy link
Member Author

FagnerMartinsBrack commented May 28, 2016

@impress/mergers @Pierstoval Ready for the next round of review.

I think we should be conservative on what to document. If we document something, then we need to take care not to break it in a new release, and since the tests are too weak there is no ensurance that it will not break.

The best thing to do here is document only what is being used in the wild. The best thing to do is to look for some forks and see how they are using impress.js. If there is a considerable amount of usage and the code comments mention that, then we should document it in the public API.

Does it makes sense?

Ping @bartaz


#### 3D Coordinates Positioning (data-z)

Define the pixel based position in which the **center** of the [Step Element](#step-element) will be positioned inside the infinite canvas on the third dimension (Z) axis. For example, if we use `data-z="-3000"`, it means that the [Step Element](#step-element) will be positioned far away from the camera (by 3000px).
Copy link
Member Author

@FagnerMartinsBrack FagnerMartinsBrack May 29, 2016

Choose a reason for hiding this comment

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

I am using the concept "positioned inside the infinite canvas" when we talk about the actual element that is positioned despite the presentation having been started or not. When we talk about perception of positioning inside the presentation flow, I use "positioned from the camera".

@bartaz
Copy link
Member

bartaz commented Jun 7, 2016

@FagnerMartinsBrack

Sorry for keeping you that long without any answer. I added couple of inline comments. But these are not big things.
I think these docs are already in a very good state and could be merged. We can work on the details later on. For sure it doesn't seem that there is anything wrong with the docs that would make something confusing or not working as expected.

@FagnerMartinsBrack
Copy link
Member Author

FagnerMartinsBrack commented Jun 16, 2016

@bartaz

Sorry to take this longer to respond, I have been in a crazy marathon of events, meetups and trips this past few weeks.

I will handle those issues probably this weekend and merge it. In the meantime, a follow-up with some suggestion I have made there (to see if they make sense) would be much appreciated.

@bartaz
Copy link
Member

bartaz commented Jun 16, 2016

@FagnerMartinsBrack it's nothing compared to my response time, so no worries ;)

@FagnerMartinsBrack
Copy link
Member Author

I think these docs are already in a very good state and could be merged. We can work on the details later on. For sure it doesn't seem that there is anything wrong with the docs that would make something confusing or not working as expected.

Landing on master then...

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.

3 participants