add slide separator in case no separator is provided and heading indicates the new slide #9

PrajwolAmatya · 2024-07-04T08:58:15Z

Description

In this PR, adds a new attribute value data-separator-by-heading to indicate whether heading represents the start of the new slide. In this case, no slide separator is required and the plugin will add default slide separator above the slide headings.

Usage

To use this feature, add the attribute data-separator-by-heading in the section tag as below:

<div class="reveal">
    <div class="slides">
        <section data-markdown="path/to/markdown/file" data-seperator-by-heading></section>
    </div>
</div>

And the markdown file can be simply provided as:

---
metadata_key1: metadata_value1
metadata_key2: metadata_value2
---
# Slide 1

# Slide 2

# Slide 3

plugin/awesoMD/plugin.js

individual-it

The description seems not to describe everything that happens in the PR, or is it because its mixed with #8 ?

individual-it · 2024-07-14T13:19:41Z

plugin/awesoMD/plugin.js

+
+		// add slide separator in the case heading indicates the new slide
+		if (options.separateByHeading) {
+			options['slideSeparator'] = '---'


still, this is "hardcoded"
what will happen if the app options.separator is set to +++ but we add --- as a separator here?

the apps options.separator doesn't store the separator value, instead it stores the regex pattern of the slide separator. The default regex pattern is stored as const DEFAULT_SLIDE_SEPARATOR = '\r?\n---\r?\n'. If other pattern is to use then it is defined by attribute data-separator in the index.html file as:
<section data-markdown="markdown/simplified.md" data-separator="\r?\n---\r?\n"></section>

The slide separator used in the markdown file and the pattern defined in data-separator should match. The plugin looks for the slide separator pattern and separates the slide. If different value is used for the slide separator in the markdown file then we should provide that regex pattern in data-separator attribute, and if this attribute is not specified in the section tag then the plugin will take the pattern specified in DEFAULT_SLIDE_SEPARATOR.

In our case, we do not provide slide separator in markdown file, so we somewhere need to specify the value of the slide separator. If we don't want to hard code here in the plugin, we can set this as default slide separator and specify the slide separator in index.html file as:
<section data-markdown="markdown/simplified.md" data-separator="\r?\n---\r?\n" data-separator-by-heading slide-separator="---"></section>
When there is data-separator-by-heading attribute, there should be slide-separator present in the section tag and matching regex pattern in data-separator attribute. With that we can skip the hard coding slide separator in the plugin.

OK, thank you for the explanation 👍
I now understand the challenge.
I rather would not introduce a new option. Maybe the easiest is to define that when data-separator-by-heading is used data-separator cannot be set to anything that is not default.
So then we can keep it hard-codded, but just need to have a check when options.separateByHeading is set whether options.separator === DEFAULT_SLIDE_SEPARATOR and if not show an error

individual-it · 2024-07-14T13:23:10Z

tests/unit/jest/extractMetadata.spec.js

+        const spySeparateInlineMetadataAndMarkdown = jest.fn(plugin().separateInlineMetadataAndMarkdown)
+        const [markdown, fm] = plugin().parseFrontMatter(markdownContent, {})
+
+        const slides = markdown.split(/^---\n/gm)


please create a test with an other separator also, see my comment above

individual-it · 2024-07-14T14:13:47Z

plugin/awesoMD/plugin.js

+		const newMetadata = headingWithMetadataRegex.test(markdown)
+		yamlRegex.lastIndex = 0
+
+		if (options.separateByHeading) {


please add a test that checks if the behavior is correct when this value is set to true

tests/unit/testFiles/inlineMetadata.md

PrajwolAmatya · 2024-07-15T08:21:58Z

The description seems not to describe everything that happens in the PR, or is it because its mixed with #8 ?

Yes, this PR is rebased with #8, that's why the description does not match. First I want to merge that PR and then this PR.

…cates the new slide

…arator-by-heading is set to true

individual-it

last commit should also have a test, rest looks good to me

done

nirajacharya2 · 2024-07-25T04:29:26Z

plugin/awesoMD/plugin.js

+                }
+            }
+
+            if (animateLists === true) {


Suggested change

if (animateLists === true) {

if (animateLists) {

Is the type of animatedList ever checked? If not then I don't think this is good style, because this will be also true for these cases:

animateLists='false'

animatedList=2

Also how does this change relate to the topic of the PR?

These are the codes that we copied from the main reveal.js plugin. The animateLists variable is true if defined otherwise it is undefined.
This is not actually related to this PR. Maybe the confusion came up because there's a lot of changes in the plugin.js file.
I will revert this change back, since it is the part of code that was copied from reveal.js plugin. 👍

…ided with data-separator-by-heading

PrajwolAmatya self-assigned this Jul 4, 2024

PrajwolAmatya force-pushed the add-separator branch from c37b6f8 to 1ace1cb Compare July 4, 2024 09:12

PrajwolAmatya marked this pull request as ready for review July 4, 2024 10:14

PrajwolAmatya requested review from individual-it, saw-jan and nirajacharya2 July 4, 2024 10:14

PrajwolAmatya force-pushed the add-separator branch 2 times, most recently from cda8d47 to 8a62791 Compare July 5, 2024 10:55

nirajacharya2 reviewed Jul 9, 2024

View reviewed changes

plugin/awesoMD/plugin.js Outdated Show resolved Hide resolved

individual-it previously requested changes Jul 11, 2024

View reviewed changes

plugin/awesoMD/plugin.js Outdated Show resolved Hide resolved

plugin/awesoMD/plugin.js Outdated Show resolved Hide resolved

individual-it mentioned this pull request Jul 11, 2024

update plugin to support new inline metadata #8

Merged

PrajwolAmatya force-pushed the add-separator branch 2 times, most recently from 652c263 to 3c1659e Compare July 12, 2024 08:58

individual-it reviewed Jul 14, 2024

View reviewed changes

PrajwolAmatya force-pushed the add-separator branch 3 times, most recently from c16ff3d to ade9262 Compare July 24, 2024 08:32

PrajwolAmatya requested review from nirajacharya2 and individual-it July 24, 2024 08:33

PrajwolAmatya added 4 commits July 24, 2024 14:42

added unit test for separeteInlineMetadataAndMarkdown functionality

dd924f4

add slide separator in case no separator is provided and heading indi…

5485f68

…cates the new slide

add unit test

bce6823

define functions as method property and add unit tests

16ace15

PrajwolAmatya force-pushed the add-separator branch from ade9262 to 16ace15 Compare July 24, 2024 09:05

throw an error on browser if data-separator is provided when data-sep…

60f69f3

…arator-by-heading is set to true

individual-it reviewed Jul 24, 2024

View reviewed changes

individual-it self-requested a review July 24, 2024 12:40

nirajacharya2 reviewed Jul 25, 2024

View reviewed changes

PrajwolAmatya force-pushed the add-separator branch from 85d6bda to 23cad6f Compare July 25, 2024 05:37

nirajacharya2 approved these changes Jul 25, 2024

View reviewed changes

add unit test for showing error message in case data-markdown is prov…

f4fb43c

…ided with data-separator-by-heading

PrajwolAmatya force-pushed the add-separator branch from 23cad6f to f4fb43c Compare July 25, 2024 08:31

PrajwolAmatya merged commit e13ee8a into main Jul 25, 2024
1 check passed

PrajwolAmatya deleted the add-separator branch July 25, 2024 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add slide separator in case no separator is provided and heading indicates the new slide #9

add slide separator in case no separator is provided and heading indicates the new slide #9

PrajwolAmatya commented Jul 4, 2024

individual-it left a comment

individual-it Jul 14, 2024

PrajwolAmatya Jul 15, 2024

individual-it Jul 16, 2024

individual-it Jul 14, 2024

individual-it Jul 14, 2024

PrajwolAmatya commented Jul 15, 2024 •

edited

Loading

individual-it left a comment

nirajacharya2 Jul 25, 2024

individual-it Jul 25, 2024

PrajwolAmatya Jul 25, 2024

add slide separator in case no separator is provided and heading indicates the new slide #9

add slide separator in case no separator is provided and heading indicates the new slide #9

Conversation

PrajwolAmatya commented Jul 4, 2024

Description

Usage

individual-it left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrajwolAmatya commented Jul 15, 2024 • edited Loading

individual-it left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PrajwolAmatya commented Jul 15, 2024 •

edited

Loading