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

Added incremental reveal #13

Closed
wants to merge 9 commits into from
Closed

Added incremental reveal #13

wants to merge 9 commits into from

Conversation

pci
Copy link

@pci pci commented Jul 26, 2011

Paul,

Love dzslides, but quite needed the powerpoint style reveal one-by-one. So I added it.

It can be styled by three classes:
incremental - nodes to be shown
current - node most recently shown
incremented - nodes that have be shown

I've change slide three of template.html to give an example.

Also onstage.html had to be tweaked to send the "forward()" and "back()" messages to the remote view, not just local.

Thanks for the great resource,
Phil Ingrey

function setSlide(i) {views.present.postMessage("setSlide(" + i + ")", "*"); }
function back() {views.present.postMessage("back", "*");views.remote.postMessage("back", "*"); }
function forward() {views.present.postMessage("forward", "*");views.remote.postMessage("forward", "*"); }
function setSlide(i) {views.present.postMessage("setSlide(" + i + ")", "*");}
Copy link
Owner

Choose a reason for hiding this comment

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

Why do need to do that? I don't think you need to.

Copy link
Author

Choose a reason for hiding this comment

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

I put the incremental code in the forward/back functions within template.html, but the onstage.html only triggered the "present" view's functions and not the "remote" view's

Copy link
Owner

Choose a reason for hiding this comment

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

  1. you assume views.remote exists. You should not.
  2. if you do that, then you should remove the initial remote.postMessage(setSlide)

I think it would be better to keep the current behavior but to extend to notion of "idx".
Maybe it could be a string instead of a number, that could look like that:

4.3.8 (slide number 4, 3rd element of the 1st level of "incrental" element, 8th element of the 2nd level of "incrental" element, ...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I had a similar idea but I don't understand why you would want n parts in this system.

In my mind there's just back and forward so one number for slide idx and one number for "inslide" step should be enough.

Incremental elements could be identified as a ordered list that can be revealed one at a time.

Copy link
Author

Choose a reason for hiding this comment

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

So you could post a command setSlide(slideNumber, incrementNumber (optional))? Sounds like a good idea, the only problem would be making sure views.future is always one ahead, but I guess you want it showing the next slide, not the next bullet point so setSlide(slideNumber+1) would work fine.

My initial reason for adding these lines was because onstage.html only posts forward()/back() commands to views.present, and then only when views.present changes slide does it post (a setSlide() command) to views.future/views.remote. Wouldn't it be simpler to just send any forward()/back() command sto all views currently connected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I totally agree with you Philip, the way present and remote views are handled is strange. They should be synchronized and receive back/forward commands. Present should send message to onstage containing details of current slide.

About future view, I think it just needs to display next slide without changing steps. Maybe displaying the slide like if it were already incremented to the end...

Present view could also inform you how many steps remains before next slide...

Copy link
Author

Choose a reason for hiding this comment

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

Re-doing the communication is really a separate, much bigger, question. I'll open a new issue on it.

@pci
Copy link
Author

pci commented Jul 29, 2011

I've added a test implementation of .incremental-list. .incremental-skip might still serve a purpose, but it would need to be well defined, e.g. would the iterator jump over the item making it and the next entry visible? But then which items would be highlighted?

@hsablonniere
Copy link
Collaborator

I didn't understand that you wanted to keep .incremental for ul/ol and .incremental-list for other container elements. It looks strange to me. We should have :

  • a class for elements that needs to be revealed : .incremental
  • a class for elements that needs to reveal their direct children one at a time : .incremental-list
  • maybe a class for skipping elements in a .incremental-list element

I still think that sortIncrement() is not needed and that we can retrieve all elements that needs to be incremented. Did you have a look at my implementation ?

Have a look at my commit (hsablonniere/dzslides@2ae40b35db997c1c05cb) and tell me what you think...

@pci
Copy link
Author

pci commented Aug 2, 2011

I kept them so that, currently, both could be experimented with.

I like your way of doing it, it really comes down to a personal preference. I did it my way as the css is more user friendly (no ":not" selectors) and the javascript CSS query is simpler so might be very slightly quicker, I realise I'm probably gaining less than a faction of a millisecond....

The disadvantages of my method (and the advantages of yours) are that I have to have this one off function added in at the start.

@hsablonniere
Copy link
Collaborator

We could flip a coin ;-)

More seriously, I don't really mind which implementation we use if we have the same result working like I described earlier.

Maybe Paul could arbitrate...

@paulrouget
Copy link
Owner

I'm kind of lost in this discussion :)
I see a lot of comments everywhere (pullrequest, inline in the patches,
in the issues, ...). Can we have a linear discussion somewhere?

@pci
Copy link
Author

pci commented Aug 3, 2011

I guess it is getting a little crowded :)

Issue #9 seems like good neutral ground to discuss it, I'll try and condense my method and put a post there, and if hsablonniere does the same that'll be a good starting place.

@paulrouget
Copy link
Owner

Works for me.

Hubert, can you summarize your work there as well?

BTW, thank you guys to take the time to fix this issue ;)

@hsablonniere
Copy link
Collaborator

Sorry for the mess, I'm kinda lost too :p

Summary : done!

@paulrouget
Copy link
Owner

DUP issue #9. Please attach future pull-request to issue #9.

@paulrouget paulrouget closed this Aug 6, 2011
This pull request was closed.
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