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

Attempt to consolidate admon content model #60 #67

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

tomschr
Copy link
Contributor

@tomschr tomschr commented Mar 7, 2019

This PR fixes #60 and contains the following changes:

  • Disallow formalpara
  • Reduce content model of admonitions in general
    => allow remark, para, lists, screen, and xi:include.
  • Add testcase

@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
@tomschr tomschr self-assigned this Mar 7, 2019
@tomschr tomschr requested a review from a user March 7, 2019 09:52
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

This seems a bit too restrictive. My main tenet was that admons should not have formalpara as their first element (after title). Maybe go for something more like allowing:

title?,
remark*,
para,
(para | formalpara | all_manner_of_lists | verbatim_stuff | remark | ... )*

@ghost ghost requested a review from taroth21 March 7, 2019 11:11
@tomschr
Copy link
Contributor Author

tomschr commented Mar 7, 2019

Oh, I see! I thought, you wanted to get rid of formalpara altogether. Will adapt it accordingly.

@tomschr
Copy link
Contributor Author

tomschr commented Mar 7, 2019

@sknorr better? 😉

Copy link
Contributor

@taroth21 taroth21 left a comment

Choose a reason for hiding this comment

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

I would agree that we should keep formalpara within admonitions but not as first element.

@ghost
Copy link

ghost commented Mar 7, 2019

Thanks! Looks much better. I'd suggest to allow informal elements as well, though:

(via TDG 5.1)

informalequation
informalexample
informalfigure
informaltable (db.cals.informaltable)
informaltable (db.html.informaltable)

@tomschr
Copy link
Contributor Author

tomschr commented Mar 7, 2019

Thanks! Looks much better. I'd suggest to allow informal elements as well, though:

Good. 👍 I was unsure about the informal elements, but I can add them too.

However, do we really need a table inside admonitions?
@taroth21 Do you have any use case for tables inside admonitions?
(Just curious)

informaltable (db.cals.informaltable)
informaltable (db.html.informaltable)

We don't support HTML tables, only CALS.

informalequation

We don't support that too. 😉

@taroth21
Copy link
Contributor

taroth21 commented Mar 8, 2019

However, do we really need a table inside admonitions?
@taroth21 Do you have any use case for tables inside admonitions?
(Just curious)

I vote against informaltables in admonitions, they tend to blow up the admonitions. :;

informaltable (db.cals.informaltable)
informaltable (db.html.informaltable)

@ghost
Copy link

ghost commented Mar 8, 2019

Please just allow all informal elements that we support. I don't really see tables as "blowing up" admons -- whatever that means in particular.

I do admit that admon/informaltable is probably a very niche use case but it might be useful, e.g for a <note/> that contains a small comparative feature table etc.

I don't think there is much potential for "abuse" by writers, especially given the complexities of table markup.

@tomschr
Copy link
Contributor Author

tomschr commented Mar 11, 2019

admon/informaltable is probably a very niche use case but it might be useful, e.g for a that contains a small comparative feature table etc.

My point on this:

  • I haven't seen a use case in our docs so far. Although this isn't argument per se against it, it is IMHO a strong indicator that we don't need that (at least not now).
  • Tables inside admons make layout problems. Usually admons can be indented when used in list. An admon with tables makes it even more harder, especially on mobile devices (EPUB or HTML).
  • From my perspective, ideally an admon should contain only some text, not feature tables. These should go outside of the admon. However, that is my personal opinion/taste and others may not agree with that.

I don't think there is much potential for "abuse" by writers, especially given the complexities of table markup.

Maybe not "abuse", but we also should guide our writers what we think is appropriate in our documentation and what is not.

What about the following idea: we try to work without informaltables in admons first. If we find out, that it is too restrictive, I will happily add them. Deal? 😉

* 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
* 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
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 this pull request may close these issues.

formalpara can be used as first element of an admon
2 participants