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

paginate: skip directive #359

Merged
merged 6 commits into from
Jun 6, 2023
Merged

Conversation

JaninaWibker
Copy link
Contributor

@JaninaWibker JaninaWibker commented May 29, 2023

This feature has been proposed and added to the roadmap here but not implemented as of yet.

As I stumbled across this and couldn't replicate it using custom directives I went ahead and tried to implement it myself.

In addition to paginate: skip I also added paginate: hide-and-skip which is a combination of paginate: false and paginate: skip, thus allowing you to do the following:

  • show pagination, increment pagination (paginate: true)
  • hide pagination, increment pagination (paginate: false)
  • show pagination, don't increment pagination (pagination: skip)
  • hide pagination, don't increment pagination (pagination: hide-and-skip)

The addition of hide-and-skip wasn't part of the original proposal, I therefore split it up into a separate commit which can easily be deleted if the feature isn't wanted.

I added 2 tests and updated the docs already, the wording might still need some improvements however.

I'm open for feedback!

@yhatt yhatt self-assigned this May 30, 2023
@yhatt yhatt self-requested a review May 30, 2023 04:25
@yhatt yhatt removed their assignment May 30, 2023
@yhatt
Copy link
Member

yhatt commented May 31, 2023

Thank you for your contribution and making me remind the issue #218.

In the original proposal, paginate: skip meant "Hide page number, and don't increment it". This behavior is necessary and sufficient to solve an original problem: Exclude the title page from the total number of pages.

Your implementation of paginate: skip seems to change the meaning of "skip" at first glance: Don't increment the page number but still show it. I have no idea of a use case where this behavior makes sense. Do you have a useful use case for this when creating slides?

In any case, I think show-and-hide is a bit long words. I think taking more simple word is better to help writing slides. IMO hold: Keep showing and holding the same page number in the current and following slide pages. In short 👇

paginate Page number Increment
true Show Yes
false Hide Yes
hold Show No
skip Hide No

I'm not a native speaker, so please let me know if my choice of words was unnatural.

Another idea: YAML object

An another idea is allowing YAML object for setting detailed option for paginate directive.

paginate:
  show: false
  increment: false

It's clear intentions for each field of YAML object. However, this may result in more lines of Markdown by directives. I think taking shorter words will be appreciated by Marp users.

@JaninaWibker
Copy link
Contributor Author

JaninaWibker commented May 31, 2023

Oh yeah right I did mess up the meaning of skip. I'll update it to your suggestion of using skip and hold 👍 (naming it hold is a great idea; did not think of that)

Don't increment the page number but still show it. I have no idea of a use case where this behavior makes sense. Do you have a useful use case for this when creating slides?

My use case is "animations", I have a series of images which change slightly from one to the next (one image per slide; figures which each highlight a different part of a plot) and would like to have the same page number for all of these.
Another use case is having multiple slides with bullet points being added slide-by-slide.
I think it makes sense to not change the page number in those circumstances.


Are you otherwise happy with the PR? 😅

changes `hide-and-skip` to `skip`
changes `skip` to `hold`
updates the documentation regarding pagination
@pavelzw
Copy link

pavelzw commented May 31, 2023

multiple slides with bullet points being added slide-by-slide

This can already be done with fragments.

@JaninaWibker
Copy link
Contributor Author

This can already be done with fragments.

This doesn't work for outputting pdfs as fragments get collapsed to the "fully revealed" state (which depending on the specific use case is what is wanted).

Additionally there is a small annoyance with fragments: Mixing normal list entries and fragment list entries results in multiple <ul> (i.e. separate lists) being generated:

- Bullet point A
- Bullet point B
+ Bullet point C
	- Bullet point C.1
+ Bullet point D

generates two lists, one for A and B, and one for C, C.1 and D (when fully revealed)

The issue with that is that themes need to accommodate this and remove/collapse the margin for subsequent lists


It would be great if this could be done "manually" using multiple slides without increasing the page number and total page count

@pavelzw
Copy link

pavelzw commented Jun 4, 2023

This doesn't work for outputting pdfs

Here is a workaround for that if you didn't know (from marp-team/marp-cli#496).

Copy link
Member

@yhatt yhatt left a comment

Choose a reason for hiding this comment

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

CI error has been fixed in #360. For running unit tests, could you merge the changes in the upstream branch main?

docs/directives.md Outdated Show resolved Hide resolved
test/markdown/directives/apply.js Show resolved Hide resolved
Copy link
Member

@yhatt yhatt left a comment

Choose a reason for hiding this comment

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

OK, Thank you for your patient contributions 😄

Ship with the latest v2 release, and let's include it into the downstream Marp projects.

@yhatt yhatt linked an issue Jun 6, 2023 that may be closed by this pull request
@yhatt yhatt merged commit cdd063f into marp-team:main Jun 6, 2023
12 checks passed
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.

How to except title slide from total number of pages of pagination?
3 participants