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

Issue 268 filters #269

Merged
merged 10 commits into from
Jun 23, 2023
Merged

Issue 268 filters #269

merged 10 commits into from
Jun 23, 2023

Conversation

martindholmes
Copy link
Collaborator

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:

  1. In the section of json.xsl where the user's configuration is serialized into JSON, I haven't bothered to write serialization for the custom labels themselves; I've only recorded the fact that custom labels were specified. This is because a) there will be further features added to the hcmc:filter element which may also have to be serialized, and it'll probably make sense to do all that at the same time, but also b) I'm not really sure if we have any use for the JSON serialization of the configuration file at all, so rather than add build time and complexity I've left this for the future.
  2. I've added tests for one desc filter and one boolean filter into our test suite; I think that's enough to be sure the setup is working, and adding more would require adding new filters into the test so that we have (for example) multiple date filters.
  3. I'm not sure what to do about the "search only in" filter, which is a bit different from other filters in that it doesn't have a filter name arising from a meta tag. Its caption is configurable in the caption set, which is currently limited to plain text.
  4. I haven't added any documentation for this feature yet, following our normal procedure which is to get a feature merged into dev before we add the documentation.

</xsl:copy>
</xsl:when>
<xsl:otherwise>
<label for="{$filterId}"><xsl:sequence select="$filterName"/><xsl:text>: </xsl:text></label>
Copy link
Contributor

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)

Copy link
Collaborator Author

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: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]"/>
Copy link
Contributor

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>


Copy link
Collaborator Author

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.

@joeytakeda
Copy link
Contributor

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.

@martindholmes
Copy link
Collaborator Author

I'll merge for now and raise a separate issue about the colon on the end of boolean filter labels.

@martindholmes martindholmes merged commit 5c0f2fd into dev Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants