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

Support frame attribute on choice table #2978

Merged
merged 4 commits into from May 31, 2018

Conversation

Projects
2 participants
@robander
Member

robander commented May 30, 2018

Description

  1. Moves logic for simpletable/@frame into mode templates to make the logic reusable
  2. Reuse the logic from choicetable

Motivation and Context

One of my customers noticed that @frame is ignored on <choicetable> -- all choice tables in PDF come out with no frames, no row separators, and no column separators. When I checked I noticed that the logic for supporting these on <simpletable> was overly verbose and not reusable without copying.

This pull request 1) puts the frame logic into mode templates that can be reused, and 2) calls those mode templates from the <choicetable> elements. The logic is set up so that it also works when called from the context of the actual <choicetable> context for generated headers.

Not currently adding the same support to <properties> because (unlike choice tables) it has columns that can disappear; with choice tables we know what the final column is.

How Has This Been Tested?

Created a <choicetable> with each frame value, with and without default headers. Also created a <simpletable> with each frame value. Verified that the simple table appears the same before and after the fix, and verify that the frame values operate the same in equivalent choice tables.

Sample files:
choicesample.zip

Type of Changes

Minor enhancement to simple tables to make logic reusable; bug fix for choice tables.

robander added some commits May 30, 2018

Consolidate frame logic in reusable templates
Signed-off-by: Robert D Anderson <robander@us.ibm.com>
Support frame attribute on choicetable
Signed-off-by: Robert D Anderson <robander@us.ibm.com>
</xsl:otherwise>
</xsl:choose>
</xsl:variable>
<xsl:variable name="frame" as="xs:string" select="if (../@frame)

This comment has been minimized.

@jelovirt

jelovirt May 30, 2018

Member

This could be even more terse as (../@frame, $table.frame-default)[1] but I don't know if we want to start using a pattern like that. In e.g. JS you can do a || b for the same thing and IMO it's more readable.

</xsl:otherwise>
</xsl:choose>
</xsl:variable>
<xsl:variable name="frame" as="xs:string" select="if (ancestor::*[contains(@class, ' topic/simpletable ')][1]/@frame)

This comment has been minimized.

@jelovirt

jelovirt May 30, 2018

Member

Related to the previous comment, in a case like this where the first expression is long, the short circuit is nicer

(ancestor::*[contains(@class, ' topic/simpletable ')][1]/@frame, $table.frame-default)[1]

then (ancestor-or-self::*[contains(@class, ' topic/simpletable ')][1]/@frame)
else ($table.frame-default)"
as="xs:string"/>
<xsl:if test="$frame = 'all' or $frame = 'topbot' or $frame = 'top'">

This comment has been minimized.

@jelovirt

jelovirt May 30, 2018

Member

$frame = ('all', 'topbot', 'top') is easier to read.

This comment has been minimized.

@robander

robander May 30, 2018

Member

👍 didn't even think about refactoring that one - it was just copy/paste of the existing logic.

Simplify use based on code review
Signed-off-by: Robert D Anderson <robander@us.ibm.com>
@robander

This comment has been minimized.

Member

robander commented May 30, 2018

Updated / simplified all of the $frame declarations and references based on the review comments (including those for <table>, which didn't have any other logic changes for this pull request).

<xsl:template match="*" mode="simpletableSideBorders">
<xsl:param name="frame" select="(ancestor-or-self::*[contains(@class, ' topic/simpletable ')][1]/@frame, $table.frame-default)[1]"
as="xs:string"/>
<xsl:if test="($frame = 'all') or ($frame = 'topbot') or ($frame = 'sides')">

This comment has been minimized.

@jelovirt

jelovirt May 30, 2018

Member

This too could be written as $frame = ('all', 'topbot', 'sides')

<xsl:template name="generateSimpleTableHorizontalBorders">
<xsl:param name="frame" as="xs:string?"/>
<xsl:choose>
<xsl:when test="($frame = 'all') or ($frame = 'topbot') or ($frame = 'sides') or not($frame)">
<xsl:when test="$frame = ('all', 'topbot', 'sides') or not($frame)">

This comment has been minimized.

@jelovirt

jelovirt May 30, 2018

Member

Instead of not($frame) we should use empty($frame)

Additional code review changes around
Signed-off-by: Robert D Anderson <robander@us.ibm.com>

@jelovirt jelovirt merged commit ead55c6 into dita-ot:develop May 31, 2018

2 checks passed

DCO All commits have a DCO sign-off from the author
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jelovirt jelovirt added this to To Do in 3.1 via automation May 31, 2018

@jelovirt jelovirt added this to the Next milestone May 31, 2018

@jelovirt jelovirt moved this from To Do to Done in 3.1 May 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment