-
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
Add sections plugin #733
base: master
Are you sure you want to change the base?
Add sections plugin #733
Conversation
86d5e5d
to
7f1c25f
Compare
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.
This looks like a nice contribution. I have suggestions regarding the user visible syntax, but like the overall idea.
Note that this PR also lacked a run of "npm run build"
src/plugins/sections/README.md
Outdated
.impress-current-section { | ||
padding-left: 5px; | ||
} | ||
``` |
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.
Please also add all of the above to https://github.com/impress/impress.js/blob/master/src/plugins/README.md#sample-html-to-enable-plugins-and-extra-addons
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.
Do you mean that the example should include all of them and the section plugin?
src/plugins/sections/README.md
Outdated
``` | ||
|
||
The sections plugin supports also sub section. Therefore, add the attribute `data-sub-section="…"` to your subsection. | ||
The agenda list and also the section overview will show the sub sections. |
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.
Hmm...
Now I'm thinking maybe nested divs is more appropriate for this after all?
<div id="impress">
<div data-section="Intro">
<div id="title" class="step title">...</div>
<div id="agenda" class="step"><div class="section-agenda"></div></div>
</div>
<div data-section="Historical context">
<div data-section="Initial idea">
<div class="step">...</div>
<div class="step">...</div>
</div>
<div data-section="First version">
<div class="step">...</div>
<div class="step">...</div>
<div class="step">...</div>
</div>
</div>
<div data-section="Next top level section">
This would allow for an arbitrary depth of sections but with the advantage of not introducing a new keyword for each level!
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.
@henrikingo, what do you think about using the <section>
tag:
<div id="title" class="step">
<h1>Title of Slide</h1>
<ul id="impress-section-agenda"></ul>
</div>
<section>
<h1>Section Title</h1>
<div id="first-slide" class="step">
</div>
<section>
<h1>Sub Section Title</h1>
<div id="second-slide" class="step">
</div>
</section>
</section>
BTW: sorry for the delay on this PR.
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.
Yes. Thank you for being aware of the html standard!
I wonder if the <h1>
outside a step div will work though. Of course, they can be made invisible with CSS, but even then. It seems like
<section data-title="Section Title">
Is more what we want here?
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.
Hmm... I was looking into the <header>
element too, but it still expects to have a <h1>
inside of it and will render its contents normally. So would make syntax more complicated without apparent benefit. (Just thinking out loud here.)
- Use section and h1 tags to assign steps to sections - Optional divs showing the section progress - active-section and hidden-section are classes to mark steps as active and hidden section element.
|
||
Additionally, the plugin will generate an agenda outline for you if you want it to. Therefore, add a `ul` tag with the | ||
id `impress-section-agenda` to any of your divs of the presentation (as shown in the aforementioned HTML snippet). | ||
|
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.
Automatic table of contents is cool! Thanks.
src/plugins/sections/README.md
Outdated
``` | ||
|
||
The sections plugin supports also sub section. Therefore, add the attribute `data-sub-section="…"` to your subsection. | ||
The agenda list and also the section overview will show the sub sections. |
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.
Hmm... I was looking into the <header>
element too, but it still expects to have a <h1>
inside of it and will render its contents normally. So would make syntax more complicated without apparent benefit. (Just thinking out loud here.)
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.
Remaining polish:
- Please update js/impress.js by running
npm run build
- Please use sections in
examples/classic-slides/index.html
. Use all bells and whistles, such as automatic agenda. - As part of previous point, add necessary CSS to
css/impress-common.css
- Also add example HTML (such as the automatic agenda) to https://github.com/impress/impress.js/blob/master/src/plugins/README.md#sample-html-to-enable-plugins-and-extra-addons
const currentSection = document.querySelector( | ||
"#impress-section-overview .impress-current-section" ); | ||
const sectionNumbers = document.querySelector( | ||
"#impress-section-overview .impress-section-numbers" ); |
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.
You should set these in the impress:init
event. Some other plugin/js might generate html dynamically before this plugin.
} | ||
return n; | ||
} | ||
|
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.
Code from here to the end of the function would benefit from comments.
hidden section element.