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

Allow multiple data-substep-order with the same value #825

Conversation

fnogatz
Copy link
Contributor

@fnogatz fnogatz commented Nov 20, 2022

In #779, the ability to add the order of substeps using the data-substep-order has been added. The current implementation relies on the assumption that all the given numbers are unique: a sequence 1, 2, 10, 100 is correctly handled, but another 1, 1, 2, 2 is not. I find the latter very useful to show multiple elements of different DOM nodes in a single substep.

Take for instance a slightly modified version of @codesections' original example from #779 (comment).

 <ul>
     <li class="substep"  data-substep-order="0">
         TypeScript <span class="substep" data-substep-order="3">(Microsoft)</span>
     </li>
     <li class="substep"  data-substep-order="1">
         Golang <span class="substep" data-substep-order="3">(Google)</span>
     </li>
     <li class="substep data-substep-order="2">
         Rust<span class="substep" data-substep-order="3">(Mozilla et al.)</span>
     </li>
 </ul>

In contrast to the original example, the examples are shown in a single, last substep.

This PR adds the correct handling of multiple data-substep-order with the same value. I don't think it needs to be explicitly stated in the plugin's README – as I originally thought it would already be supported :)

A real-world example that uses the proposed change can be found at multiple occasions in the slides of my PhD defense (source code).

Copy link
Member

@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 Falco, and if this is your first PR, welcome to the project!

This is a straightforward and simple addition, yet as you explain, can be very useful. Thanks for creating it.

I just have a request to add code comments, after that we can merge it.

src/plugins/substep/substep.js Show resolved Hide resolved
src/plugins/substep/substep.js Show resolved Hide resolved
@henrikingo henrikingo merged commit 7817f12 into impress:master Dec 23, 2022
@fnogatz fnogatz deleted the allow-multiple-substeps-with-same-order-number branch December 26, 2022 10:34
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.

2 participants