Skip to content

Conversation

@somiaj
Copy link
Contributor

@somiaj somiaj commented Dec 27, 2025

Apply the spacing between paragraphs added by the Par block in in HTML to the previous block instead of inserting an empty div with a margin. This adds the margin from the Par block to the previous Indent div (including adding a div around an indent of zero), list, or list item block.

Based on the code from @dpvc in #1355.

Unsure if this should be extended to any other blocks.

@somiaj somiaj force-pushed the pgml-add-par-to-prev-block branch 2 times, most recently from 8a1ff45 to 5743412 Compare December 27, 2025 03:13
Comment on lines 1274 to 1276
for my $i (0 .. $n - 1) {
my $item = $stack->[$i];
my $next_par = $i + 1 < $n && $stack->[ $i + 1 ]{type} eq 'par' ? $stack->[ $i + 1 ] : undef;
Copy link
Member

@drgrice1 drgrice1 Dec 27, 2025

Choose a reason for hiding this comment

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

Change this to

Suggested change
for my $i (0 .. $n - 1) {
my $item = $stack->[$i];
my $next_par = $i + 1 < $n && $stack->[ $i + 1 ]{type} eq 'par' ? $stack->[ $i + 1 ] : undef;
for my $i (0 .. $#$stack) {
my $item = $stack->[$i];
my $next_par = $i < $#$stack && $stack->[ $i + 1 ]{type} eq 'par' ? $stack->[ $i + 1 ] : undef;

and delete my $n = scalar(@$stack); above (line 1271). Perl is optimized for this and stores this number as a property of the array. So there is no reason to save the array length to yet another location. Also, perl offers the $# syntax for getting the index of the last entry in an array, so use that rather than adding or subtracting one from the array length.

Apply the spacing between paragraphs added by the Par block in
in HTML to the previous block instead of inserting an empty div
with a margin. This adds the margin from the Par block to the
previous Indent div, list, or list item block.

Based on the code from @dpvc in openwebwork#1355.
@somiaj somiaj force-pushed the pgml-add-par-to-prev-block branch from 5743412 to fc008f1 Compare December 27, 2025 15:27
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

This looks good.

I think that the essential thing is that the divs that were added between list items be fixed, since that is invalid HTML. Adding the style to other block items (such as the lists themself) instead of injecting an empty div after it is not particularly important since that is at least valid HTML. I am not entirely certain what benefit that provides either.

@somiaj
Copy link
Contributor Author

somiaj commented Dec 27, 2025

I am unsure on the benefit as well, but I do feel the empty divs for spacing is not ideal. I was also wondering if there were any accessibility issues with not using <p> tags or grouping text. But to me the PGML output is doesn't feel correct.

Paragraph one.
<div style="margin-top:1em"></div>
Paragraph two.
<div style="margin-top:1em"></div>
Paragraph three.

And this just feels better to me. But I couldn't figure out a way to do this, and assume that if the PGML parser was setup for this, <p> tags would have just been used in the first place.

<div style="margin-bottom:1em">
Paragraph one.
</div>
<div style="margin-bottom:1em">
Paragraph two.
</div>
<div style="margin:0">
Paragraph three.
</div>

@drgrice1
Copy link
Member

There is nothing wrong with using an empty div with a margin for spacing. Your second example does not add anything semantically. Furthermore, attempting to wrap the paragraph could cause structural problems, because it could result in something the author added not being terminated properly. Simply put, the empty div tag is less likely to cause problems, and works fine.

Using <p> tags really wouldn't work. Not only does that have the same problems as attempting to wrap with a div tag, but the content might have something in it that is not valid inside a <p> tag. There are way to many things that are not valid in a <p> tag. Even answers are not valid in a <p> tag anymore.

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