-
Notifications
You must be signed in to change notification settings - Fork 22
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
Issue 268 filters #269
Issue 268 filters #269
Conversation
… on handling them in the config processing.
… debugging to do.
</xsl:copy> | ||
</xsl:when> | ||
<xsl:otherwise> | ||
<label for="{$filterId}"><xsl:sequence select="$filterName"/><xsl:text>: </xsl:text></label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, the colon after the label string is new; I don't think it's necessary (plus it's easy to add in CSS if one wants it, but much harder to remove if they don't)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the colon is there to match what we currently do already with boolean filter labels. I don't particularly like it, but I figured removing it should be a separate ticket.
xsl/makeSearchPage.xsl
Outdated
<xsl:message expand-text="yes">Filter name: {$filterName}; filter id: {$filterId}; filters to check: {count($filterLabels)}</xsl:message> | ||
<xsl:choose> | ||
<xsl:when test="$filterLabels[@filterName=$filterName and (@lang=$pageLang or not(@lang))]"> | ||
<xsl:variable name="currLabel" select="$filterLabels[@filterName=$filterName and (@lang=$pageLang or not(@lang))][1]/*[1]"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we can't specify this directly? (And maybe move the variable up to use for the <xsl:when>
? E.g.
<xsl:variable name="currLabel"
select="$filterLabels[@filterName = $filterName and (@lang = $pageLang or not(@lang))][1]/xh:label[1]"
as="element(xh:label)?"/>
<xsl:choose>
<xsl:when test="$currLabel">
<!-- ... -->
</xsl:when>
</xsl:choose>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, reworked to follow that model. Also I tested on Windows and the build worked and produced a working test site. Once it a while it's good to do that.
Thanks @martindholmes for this — tested locally with no issues at all. A few minor queries for your consideration, but otherwise looks good to me—feel free to merge whenever you think. |
I'll merge for now and raise a separate issue about the colon on the end of boolean filter labels. |
This pull request forms the initial part of work on issue #268, and implements the enhancement in issue #248, allowing users to specify custom HTML label elements including other HTML markup as replacements labels for their filters. A few points to make: