Support task Element · Pull Request #345 · openSUSE/suse-xsl

ghost · 2017-09-22T16:58:12Z

PR, mainly for the purposes of reviewing this... There must be some reason why you named the commits "first draft", I guess.

TODO: * CSS rules need to be improved, but the structure should be there * Language files; currently only "en" is supported for a proof of concept

TODO: * spaces between the (automated) title taskprerequisites and taskrelated needs to be adapted

ghost

so mostly, stuff comes out as it should but there is still lots of positioning issues related to the headlines of "tasksummary" and "taskprerequisites". can you try position these similarly to normal preambles in fo? also, in fo, there is a huge gap after the tasksummary, that really needs fixed. (i can take care of the css for html.)
why does taskrelated not get a headline in html but does get one in fo? curiously, tasksummary does not get a headline in fo but does get one in html.
i have a question that relates to geekodoc: why is it not allowed put itemizedlists in taskprerequisites? lists seem a natural fit for prereqs, no?

ghost · 2017-09-22T17:09:11Z

suse2013/common/l10n/en.xml

   <l:gentext key="step.optional" text="(Optional)"/>
+
+   <l:gentext key="tasksummary" text="Summary"/>
+   <l:gentext key="taskprerequisites" text="Requirements"/>


hm, looking at https://github.com/docbook/xslt10-stylesheets/blob/master/gentext/locale/en.xml#L350 , I wonder: are the tasksummary/taskprerequisite/taskrelated elements supposed to have actual titles instead of something autogenerated? does geekodoc allow titles for these elements?

also, (stating the obvious)make sure to add some more languages...

I wonder: are the tasksummary/taskprerequisite/taskrelated elements supposed to have actual titles instead of something autogenerated? Does geekodoc allow titles for these elements?

Currently, no title inside of these task* elements are allowed in Geekodoc. So yes, that's a feature and not a bug. 😉

Sure, we could allow titles, but I think in that case we would only get further inconsistencies which would "weaken" our task appearance. At the moment, I can't think of a use case where a title in these task* elements is absolutely necessary.

To summarize it, I would like to keep it that way for the time being. Let our writers gain some experiences. If we think we need it, we can add titles in a later version without any compatibility problems.

ghost · 2017-09-22T17:11:35Z

suse2013/static/css/style.css

 .itemizedlist-title .link, .variablelist-title .link, .qandadiv-title .link,
 .title .ulink, .subtitle .ulink, .table-title .ulink, .example-title .ulink,
-.figure-title .ulink, .procedure-title .ulink, .orderedlist-title .ulink,
+.figure-title .ulink, .procedure-title .ulink, .task-title .ulink, .orderedlist-title .ulink,


oh my. i can't even look at this css. totally not your fault but mine. but i almost feel like opening a bug about generating fewer of those class names.

to be clear: your changes are fine and should work. this is more of a note-to-past-self.

ghost · 2017-09-22T17:15:46Z

suse2013/common/labels.xsl

+     </xsl:if>
+   </xsl:variable>
+   <xsl:choose>
+    <xsl:when test="@label"><xsl:value-of select="@label"/></xsl:when>


use two spaces for indentation please. (it seems you're switching between one and two occasionally.)

Arhg, I was too inconsistent. Could we do it later? Or what procedure do you prefer to change this?

ghost · 2017-09-22T17:30:36Z

suse2013/xhtml/task.xsl

+
+ <xsl:template match="tasksummary|taskprerequisites">
+  <div class="{local-name(.)}">
+   <h6 class="{local-name(.)}-title">


you could hardcode task-title here instead, otherwise you'll have to add lots of extra css to style these extra titles.

Ok, did that.

ghost · 2017-09-22T17:37:15Z

suse2013/static/css/style.css


-.procedure-title-wrap + .procedure-contents {
+.procedure-title-wrap + .procedure-contents,
+.task-title-wrap + .task-contents {


Not directly related to this line, but could find a good place for

.task-contents .procedure-contents { border-left: none; }

That'll remove the extra line still displayed at the side.

Ok, see commit 391f98d

Suggested by sknorr, thanks!

This should fix the missing taskrelated title

* Add missing rules for .task-title-wrap, .taskprerequisites and .tasksummary * Add rule for .task-contents and its first p child * Add missing .task-contents > * rule to adjust left positioning

tomschr · 2017-09-25T09:41:39Z

[...] still lots of positioning issues related to the headlines of "tasksummary" and "taskprerequisites".

I tried to fix them in the CSS, and it looks ok-ish. However, I guess, I did more harm than good. You should review my changes carefully. See commit f4a7b79.

why does taskrelated not get a headline in html but does get one in fo? curiously, tasksummary does not get a headline in fo but does get one in html.

This should be fixed now in commit 7fcde21, there was an inconsistency in our HTML templates for the task* elements.

i have a question that relates to geekodoc: why is it not allowed put itemizedlists in taskprerequisites? lists seem a natural fit for prereqs, no?

According to comment openSUSE/geekodoc#1 (comment) we discussed this and we came up with no lists. I can't remember why, but I guess, it is an omission.

I've opened openSUSE/geekodoc#25.

Apart from the issues above, I found another problem in HTML (and probably in FO, too):

If you don't add a procedure title, there is no clear distinction between the content of a taskprerequisites and the following procedure. It looks like, the procedure belongs to the previous element, which is not really the case.
If you add a procedure title, the "Procedure x.y" text is rendered gray, whereas the title itself is in green. It looks a bit odd, but for the time we can keep it.

I'm not sure how to solve it, but I could think of the following solutions when the author doesn't add a procedure title:

Add a line.
Probably not the best idea, but it would work.
Add "Procedure x.y"
I would prefer this if we don't find a better solution
Moar space?
Something else?

Change task.children.title.spacing and include only space-before.* attributes

ghost · 2017-09-26T21:13:35Z

tests/dapscompare-tests/db5-procedure/xml/test.xml

 </info>

 <chapter xml:id="cha.procedure">
-  <title>Procedures</title>


Hm, did I accidentally put the test case stuff into the same commit here on Friday? If so, that was not intended...

tomschr added 2 commits September 22, 2017 18:56

First draft of task in XHTML

0fc8a4a

TODO: * CSS rules need to be improved, but the structure should be there * Language files; currently only "en" is supported for a proof of concept

First draft of task in FO

f953129

TODO: * spaces between the (automated) title taskprerequisites and taskrelated needs to be adapted

ghost commented Sep 22, 2017

View reviewed changes

Stefan Knorr and others added 4 commits September 22, 2017 19:44

Update test document for procedures to also include a task

631305a

Add .task-contents .procedure-contents

391f98d

Suggested by sknorr, thanks!

Correct HTML templates for task* elements

7fcde21

This should fix the missing taskrelated title

Fix positioning issues in CSS for task*

f4a7b79

* Add missing rules for .task-title-wrap, .taskprerequisites and .tasksummary * Add rule for .task-contents and its first p child * Add missing .task-contents > * rule to adjust left positioning

tomschr added 2 commits September 25, 2017 15:38

Fix positioning issues in FO for task*

4ccace0

Change task.children.title.spacing and include only space-before.* attributes

Add testcase for task with title procedure

a7fa6ed

ghost commented Sep 26, 2017

View reviewed changes

tomschr changed the title ~~Feature/task~~ Support task Element Sep 28, 2017

ghost force-pushed the feature/task branch from 27d4928 to a7fa6ed Compare October 16, 2017 14:34

ghost closed this Nov 29, 2019

ghost mentioned this pull request Nov 29, 2019

WIP: Support task Element #406

Draft

This pull request was closed.

Conversation

ghost commented Sep 22, 2017

Uh oh!

ghost left a comment • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomschr commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

ghost left a comment •

edited by ghost

Loading

tomschr commented Sep 25, 2017 •

edited

Loading