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

formalpara can be used as first element of an admon #60

Closed
ghost opened this issue Feb 20, 2019 · 8 comments · May be fixed by #67
Closed

formalpara can be used as first element of an admon #60

ghost opened this issue Feb 20, 2019 · 8 comments · May be fixed by #67
Assignees
Labels
bug 🐛 Something isn't working as expected geekodoc Version independent general GeekoDoc issues
Milestone

Comments

@ghost
Copy link

ghost commented Feb 20, 2019

In Geekodoc, you can currently use formalpara as the first element of an admon:

<important>
  <formalpara><title>bla</title><para>blub</para></formalpara>
</important>

or

<important>
  <title>haha jk</title>
  <formalpara><title>bla</title><para>blub</para></formalpara>
</important>

Neither construct makes any sense to me. Could we restrict this?

cf. the online-docs failure of the CAP guide, develop@4e27a3e6.

@tomschr
Copy link
Contributor

tomschr commented Mar 7, 2019

Good catch! Currently, admonitions have these content model (resolved):

db.admonition.contentmodel = db._info.title.only, 
   (db.list.blocks
     | db.formal.blocks   # db.example | db.figure | db.table | db.equation
     | db.informal.blocks   # db.informalexample | db.informalfigure | db.informaltable  | db.informalequation
     | db.publishing.blocks  # db.sidebar | db.blockquote | db.address | db.epigraph
     | db.graphic.blocks  # db.mediaobject | db.screenshot
     | db.technical.blocks  # db.procedure | db.task | db.productionset | db.constraintdef | db.msgset
     | db.verbatim.blocks  # db.screen | db.literallayout  | db.programlistingco | db.screenco  | db.programlisting | db.synopsis
     | db.bridgehead
     | db.remark
     | db.revhistory
     | db.indexterm
     | db.synopsis.blocks
     | db.admonition.blocks
     | db.anchor | db.para | db.formalpara | db.simpara
     | db.extension.blocks)+
   | db.annotation
   | db.xi.include

That would be a good opportunity not only remove formalpara, but to consolidate the content model of admonitions too. I propose the following model:

db.admonition.contentmodel = db._info.title.only, 
    (db.list.blocks
     | db.verbatim.blocks
     | db.remark
     | db.para
     | db.xi.include
     )+

I'm not entirely sure about graphics (db.informal.blocks and db.graphic.blocks) inside admonitions.

@sknorr What do you think? Does it look ok?

@tomschr tomschr self-assigned this Mar 7, 2019
@tomschr tomschr added bug 🐛 Something isn't working as expected geekodoc Version independent general GeekoDoc issues labels Mar 7, 2019
@tomschr tomschr added this to the 1.0.5 milestone Mar 7, 2019
@ghost
Copy link
Author

ghost commented Aug 15, 2019

@tomschr Not sure if I answered in another way or did not answer at all, but <formalpara/> should be allowed in general. However, as the first element after the title, it makes no sense.
Essentially, I'd leave most of the model intact, except I'd force an initial para.

What I completely removed v/ upstream are essentially just four things:

  • bridgeheads
  • formal blocks
  • nested admons
  • technical blocks -> procedure only

What I was unsure about: What are extension.blocks and anchor?

The following model would make more sense to me:

db.admonition.contentmodel = db._info.title.only,
    (db.remark?,
    (db.para
     | db.xi.include
     ),
    (db.list.blocks
     | db.informal.blocks   # db.informalexample | db.informalfigure | db.informaltable  | db.informalequation
     | db.publishing.blocks  # db.sidebar | db.blockquote | db.address | db.epigraph
     | db.graphic.blocks  # db.mediaobject | db.screenshot
     | db.technical.blocks  # db.procedure | db.task | db.productionset | db.constraintdef | db.msgset
     | db.verbatim.blocks  # db.screen | db.literallayout  | db.programlistingco | db.screenco  | db.programlisting | db.synopsis
     | db.remark
     | db.anchor | db.para | db.formalpara | db.simpara
     | db.extension.blocks)*),
   | db.xi.include

tomschr added a commit that referenced this issue Aug 16, 2019
* Allow formalparas inside admons, but not as a first child
  See issue #60 and PR #67

* Merge test from good/article-admonitions.xml in good/article-admons.xml
tomschr added a commit that referenced this issue Aug 16, 2019
* Force admons to have at least one para as first child
@tomschr
Copy link
Contributor

tomschr commented Aug 16, 2019

Thanks @sknorr for the suggestion! 👍 After looking at the pull request #67 again, your suggestions to force a para as a first child makes sense to me. Like it! 👍

I would have only a slight change/wish: Could we change the db.remark? to db.remark*? IMHO, it should be ok to use more than one remark, if necessary. Or do you have a special use case in your mind?

What I was unsure about: What are extension.blocks and anchor?

  • The db.anchor is/was used for the anchor element, but currently it's disabled. I think, I will remove it.
  • The db.extension.blocks is currently also disabled. It is a "customization hook". That means, if we (or someone else) would like to customize Geekodoc and introduce further elements into admons, it can be done with this pattern.

I observed a syntax error with your content model, so I'd propose to use this (clarified and collected the removed patterns in one location):

db.admonition.contentmodel =
      db._info.title.only,
      (db.remark*,
       (db.para | db.xi.include),
       # Removed patterns:
       # db.informalexample | db.informalfigure | db.informaltable  | db.informalequation
       # db.sidebar | db.blockquote | db.address | db.epigraph
       # db.mediaobject | db.screenshot
       # db.procedure | db.task | db.productionset | db.constraintdef | db.msgset
       # db.screen | db.literallayout  | db.programlistingco | db.screenco  | db.programlisting | db.synopsis
       (db.list.blocks
       | db.informal.blocks
       | db.publishing.blocks
       | db.graphic.blocks
       | db.technical.blocks
       | db.verbatim.blocks
       | db.remark
       | db.para | db.formalpara | db.simpara
       | db.extension.blocks
       | db.xi.include)*
      )

Would you like to review commit 0b7a1ec?

@ghost
Copy link
Author

ghost commented Oct 14, 2019

Uh, sorry for probably missing this initially.

Looks good, except: can you please enable the elements db.informalexample | db.informalfigure | db.informaltable | db.informalequation again?

@tomschr
Copy link
Contributor

tomschr commented Oct 14, 2019

Uh, sorry for probably missing this initially.

Na, I also forgot about it. 😉

Looks good, except: can you please enable the elements db.informalexample | db.informalfigure | db.informaltable | db.informalequation again?

Although I was originally against the idea of adding these elements into admons (to blow up the content), I'll add them anyway. See it in commit 1928040.
But keep in mind, db.informalequation is currently set as notAllowed. I think, we can live without an equation for the time being. 😉

tomschr added a commit that referenced this issue Oct 14, 2019
* Force admons to have at least one para as first child
* Add informal{example,figure,table,equation} to admon
  content model
* Add test cases
@ghost
Copy link
Author

ghost commented Oct 14, 2019

Also, I am a bit confused, your comment in the commit says: "Removed elements [...] db.screen" but db.verbatim.blocks is included -- or are these not the same? (I think we need screen within admons, so if my observation is correct, please just fix the comment.)

@tomschr
Copy link
Contributor

tomschr commented Oct 14, 2019

Yes, db.screen is included in db.verbatim.blocks, so screen is not removed. Don't worry. 😉

The original DocBook contained in db.verbatim.blocks: literalllayout, programlisting/programlistingco, screen/screenco and synopsis. It is a pattern which contains all so-called "verbatim" elements which preserves whitespace.

Yeah, maybe I should fix/remove/adapt the comment.

But I realized, I didn't add that as a test case (i think, that is really important). Thanks for the hint! 👍

tomschr added a commit that referenced this issue Oct 14, 2019
* Force admons to have at least one para as first child
* Allow informal{example,figure,table,equation} in admon
  content model (as suggested by Stefan)
* Disallow formalpara as a first child in admonitions
  (to avoid layout issues)
* Add test cases
@tomschr
Copy link
Contributor

tomschr commented Dec 8, 2022

Jana and I decided it's overkill and we leave it as is.

@tomschr tomschr closed this as not planned Won't fix, can't repro, duplicate, stale Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working as expected geekodoc Version independent general GeekoDoc issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant