Enforce para as first child in step

[tomschr]

This PR contains the following changes to fix #58:

• Enforce para as a first child
• Add testcase

[ghost]

• Your commit message references the wrong issue (needs to be "58" instead of "59")
• I guess this might almost be too restrictive (see below), but I am not 100% sure that my concerns here are valid.

[ghost]

In some cases, this kind of step/screen pattern may be appropriate. E.g. when you're working on just a cheat sheet or so. (Though I am not sure we want to cover cheat sheets with Geekodoc.)

[taroth21]

OK, for a cheat sheet, a step with only a screen might make sense. But for 98% of the cases within the document types we currently have in our portfolio, I would still say that a screen usually should be accompanied by a para and should not be the only element within a step.

[tomschr]

Well... I have to admit, I didn't think of cheat sheets. However, as Tanja already explained, we don't have that ATM. So I think, we shouldn't cover this use case.

I would propose to change it accordingly (e.g. enforce para as a first child). Even if you want to write cheat sheets, you could do that with an empy <para/>. I know, it's a bit more verbose, but in case we really need to write cheat sheets, we could revisit this issue.

[ghost]

1. I see your point -- essentially GeekoDoc is not really made for cheatsheets.
2. From a perspective of not breaking too many existing documents at a time, I would (still) somewhat favor allowing step/screen[1] - I am pretty sure this will be an issue with current SES, CaaS P and CAP docs.

In any case, we should not ask people to use empty <para/>s. That is just terrible practice and will only make people wonder how many other things they can work around in a similar way. It can also lead to layout issues (like empty lines).

[tomschr]

Yes, the empty <para/> wasn't a great idea.

If you favor step/screen[1] as an additional first element (the correct XPath would be step/*[1][self::screen], but anyway JFYI), wouldn't this issue be pointless? 😉

I'm a bit confused now as your suggestion makes the change kind of obsolete, right? Just to compare, this is the original definition of a step:

db.step =
    ## A unit of action in a procedure
   element step {
      db.step.attlist,
      db.step.info,
      ((db.all.blocks+,
        ((db.substeps | db.stepalternatives), db.all.blocks*)?,
        db.result?)
       | ((db.substeps | db.stepalternatives),
          db.all.blocks*,
          db.result?))

And here is the definition in GeekoDoc:

db.step =
    ## A unit of action in a procedure
    element step {
      db.step.attlist,
      # db.step.info?,
      db.remark?,
      db.para,
      ((db.step.blocks?,
        ((db.substeps | db.stepalternatives), db.step.blocks*)?,
        db.result?)
       |  ((db.substeps | db.stepalternatives),
          db.step.blocks*,
          db.result?)
      )      
    }

As you can see, it's quite similar. The only difference is db.step.blocks vs. db.all.blocks and the db.remark and db.para as the first child elements.

If we remove that, the whole pattern will be useless. 😉 I don't have anything against that. If we want to be compatible with existing documentation, I can remove the step customization. In that case, we should close this PR and the related issue.

What do you think?

[ghost]

I'm a bit confused now as your suggestion makes the change kind of obsolete, right?

Afai remember, the reason for this issue was that people (CaaSP writers specifically) were using (in)formalfigure as the first element and complaining that it does not look good. To which we/I said, "sure, but it also makes no sense to do that". I don't think screen usage was the issue we were concerned with. Then again, in a sense, you are right -- screen is not too different from figure in the sense that it does not give proper context for a step. I guess I ultimately agree that it probably should not be allowed in first position either.

[taroth21]

One reply to @sknorr 's comment, the other comments are just typos I spotted for the test cases. ;)

[tomschr]

Your commit message references the wrong issue (needs to be "58" instead of "59")

Yes, bad typo. 😢 I will tackle that when squashing the commits.