Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Expose 'impress' namespace and steps configuration objects #76

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

medikoo commented Jan 25, 2012

Summary:

  • Expose impress object on window (with just init function and steps configuration)
  • Renamed internalimpress variable into impressEl (as impress is now same as window.impress)
  • Initialize slideshow with impress.init function - it allows to adjust configuration programmatically after loading impress.js but before running impress.init
  • Expose steps configuration with impress.steps object, it's not exactly as internal steps array, it's hash object where steps are mapped by its ids. Objects held by internal steps array and exposed impress.steps hash are same objects
  • Simplify internal step object API so it's more user friendly
    • step.translate properties are now direct properties on step object
    • step.rotate object may be just a number which then internally would be used for rotation over z axis
  • Bulletproof validation of step object during initialization and operation on slides (it's needed as objects are now exposed)

Let me know if it makes sense.
When I built my last presentation I found convient to configure slides positioning separately from html file, it's just easier when you have large number of slides, and want to move them around. See: https://github.com/medikoo/impress.js/blob/medikoo-asynchronous-javascript/js/default.js (this branch have this one merged)

It would also make impress kind of pluggable. I've seen requests for some kind of automatic algorithm which can be used to position slides automatically, now it can neatly be incorporated by users if they want it.
You may then list in your documentation such auto positioning external plugins :)

Contributor

medikoo commented Jan 25, 2012

I just had second thought about it:

What I proposed is ok, only if we feel there's some use case for changing steps configuration during presentation (after initialization). I can't point now any, but I'm not that familiar with engine and possibilites, maybe it makes sense (?)

If there's no such case then this pull request does too much. I can clean it so let's say impress.init takes optional options object on which we can provide steps configuration via options.steps.
Then initial setup will work same as it's in master it will just favor configuration found in options.steps over what's in html.

Owner

bartaz commented Jan 28, 2012

Nice!

I'm quite surprised, cause it's going more or less into direction I'd like it to go :)

And I totally agree with your second comment. I don't see the point in exposing the configuration object. Options passed to init function is enough I think.

Anyway, there are quite a lot of changes inside. Validation (all these Number() and isNaN calls look scary ;), changed structure of internal objects. I haven't looked through all the code, yet.

The changes are quite big, so I wouldn't like to merge them into master directly.

Could you please switch the pull request and make in against integration branch, so we will have separate place to work on it and maybe clean it up before pushing into core code.

Thanks!

sokra commented Jan 28, 2012

@bartaz i think you may want to change the default integration branch.
you can change it in "Network" -> "Fork Queue"-> "change it"
but i may be wrong and this is not the correct option for that

Contributor

medikoo commented Jan 29, 2012

@bartaz yeah, diff shows a lot of changes, but mainly due to extra indent nesting caused by moving most of the code into impress.init.

Anyway I updated it so it's probably nicer and definitely more clean right now, and as you suggested I reverted from exposing configuration object, it doesn't make sense, now steps can only be passed via options.steps to impress.init.

According to validation, it's based on how native ES5 functions behave. If they expect number, then they always filter input argument throughNumber. Incorrect value will then become NaN, and here to not be too strict I fallback to default setting in such case. I'm checking mostly via isNaN(value) and not !value to not dismiss valid 0 value.

I cannot change target branch, so I'm closing this pull request and I'm gonna open new one for integration :)

@medikoo medikoo closed this Jan 29, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment