Accessibility: Removes hard-coded columns in home facet lists#4302
Accessibility: Removes hard-coded columns in home facet lists#4302ckaz wants to merge 8 commits intovufind-org:devfrom
Conversation
|
So, finally, here's the new proposal, branched off
Pls. let me know what you think and whether it is feasible to continue considering this. |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @ckaz -- I'll let @crhallberg comment on the actual styles before I dive into that part of the conversation, but I noticed a couple of minor things to clean up that are not related to the core of the logic.
2ed8a70 to
f7b6552
Compare
| <ul class="home-facet-list"> | ||
| <?php | ||
| // Special case: two columns for LC call numbers ... | ||
| $cssColumnClass = $field == 'callnumber-first' ? 'two-columns' : 'one-column'; // Add the CSS class logic here |
There was a problem hiding this comment.
If we're moving the column control to SCSS, we could remove this hard-coding of callnumber-first by adding the field as a class similar to how we handle page classes.
ckaz
left a comment
There was a problem hiding this comment.
Great ideas, many thanks
| <ul class="home-facet-list"> | ||
| <?php | ||
| // Special case: two columns for LC call numbers ... | ||
| $cssColumnClass = $field == 'callnumber-first' ? 'two-columns' : 'one-column'; // Add the CSS class logic here |
fcf3e6a to
3e81583
Compare
|
My only question at this point is if using column-count is sufficient, or if we should do something more rigid like CSS grid. |
3e81583 to
db9ec3b
Compare
Indeed, this looks tempting but wouldn't it break up the expected flow of the ul lists? vs. the default version with
|
|
I see! I misunderstood the list layout vs the layout of the facets. My mistake! |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @ckaz -- see below for some thoughts.
| <div class="home-facet <?=$this->escapeHtmlAttr($field) ?><?=$isHierarchy ? ' facet-tree' : ''?>"> | ||
| <h2><?=$labelHeading?></h2> | ||
| <div class="home-facet-container"> | ||
| <?php /* removed the 'callnumber-first' coding here because 'callnumber-first' is set on the 'home-facet' container, so columns can be set using SCSS */ ?> |
There was a problem hiding this comment.
I don't think we need to keep this comment here; if we're removing the hard-coded behavior, we don't need to keep a comment about its removal forever. I think people can rely on git to figure out the history if they're curious about the change.
| <?php /* removed the 'callnumber-first' coding here because 'callnumber-first' is set on the 'home-facet' container, so columns can be set using SCSS */ ?> |
There was a problem hiding this comment.
My suggestion about removing this comment was marked as resolved but not applied. I still think we can remove this comment; do you disagree?
| // Special case: two columns for LC call numbers... | ||
| $maxListLength = $field == 'callnumber-first' | ||
| ? $columnSize * 2 : $columnSize; | ||
| $maxListLength = $columnSize; |
There was a problem hiding this comment.
This change causes all the facets to revert to a single-column view. I think we still need a way to allow more values for fields where we want the double-column view. Ideally, it would be nice if the default configuration here resulted in something that looks the same as the default configuration in the current dev branch to demonstrate how to implement flexible columns.
Do you think it would make sense to support field-specific column sizes via the configuration, or am I missing/misunderstanding the point of the work so far?
And just to be clear where I'm coming from, here is how the dev branch looks:
And here is how this branch looks:
Hi @demiankatz , thank you. I'm on sick leave right now, so I'll look into the issue again in a couple of days. Cheers. |
|
@ckaz, I agree that we should implement the two-column appearance with more accessible markup -- which you have done here. My point is that in order to populate the new markup, we still need to retrieve more values for the facets that are supposed to display in two columns, and I think we need some kind of mechanism to accomplish that. In any case, I hope you feel better; thanks for taking the time to comment even while not in office (and please don't feel any urgency to respond to this before you return). |
Hi @demiankatz , yes, I am much better ;-) I'm afraid I don't quite understand what you mean with "we still need to retrieve more values for the facets that are supposed to display in two columns"? |
There is code in the template that sets a |
Ok, I hope I get this right: In this PR we have the option to display content of one ul in two columns (conformant to WCAG). So it is possible to display call numbers as a one column or two-column ul (depending on the setting chooses in the SCSS variables |
@ckaz, in the template code, there is a variable called |
There was a problem hiding this comment.
This is the accessibility problem you were pointing out, right?
There was a problem hiding this comment.
Good catch, @crhallberg, I think that code was removed in an earlier version of this PR; if it has returned, that's a problem.
Ok, thanks for clarifying. I think I get it now. |
|
@crhallberg and @demiankatz So couldn't we simply, at least as an interim solution, re-instate the doubling of the column content:
and then remove/rewrite the elseif in lines 53 to 58 so the first
This would give us the desired number of items in one |
|
Yes, @ckaz, that's more or less what I would suggest, and we can add configurability as a followup later. |
8d638d4 to
7325184
Compare
|
Sorry for the confusion, @demiankatz . Here's my new suggestion: We re-instate the extra column content for callnumber-first home facet and remove the extra |
demiankatz
left a comment
There was a problem hiding this comment.
@ckaz, I think we're on the same page about goals, but things aren't working for me yet. Here's how it looks on the dev branch:
Here's how it looks here:
...so something is preventing the second column formatting from being applied.
This observation may or may not be related to the problem, but I think it's worth considering in either case:
I notice that your SCSS has hard-coded rules in it related to callnumber-first, but you have variables for "non-default-column-count." I wonder if we can clean this up so the hard-coded assumptions are in fewer places. Since the template is currently the place where we make decisions based on the field name, can we apply a class (like "double-column") in the template and then use that in all the SCSS in place of hard-coded names? That will make it easier to make this configurable in the future, and should also make the SCSS a little shorter/more readable.
I also spotted a few minor formatting details which are listed below.
| <div class="home-facet <?=$this->escapeHtmlAttr($field) ?><?=$isHierarchy ? ' facet-tree' : ''?>"> | ||
| <h2><?=$labelHeading?></h2> | ||
| <div class="home-facet-container"> | ||
| <?php /* removed the 'callnumber-first' coding here because 'callnumber-first' is set on the 'home-facet' container, so columns can be set using SCSS */ ?> |
There was a problem hiding this comment.
My suggestion about removing this comment was marked as resolved but not applied. I still think we can remove this comment; do you disagree?
* introduces multiple column options for two-columned lists such as 'callnumber-first' with pure CSS breaks (W3C accessibility requirement) * adds various options for customization such as column-fill, different settings for XS/SM/MD devices and more * adds customizable visual division between home page facet blocks
* adds empty line before end of file in variables.scss
|
Hi @demiankatz ! You wrote:
Yes, the setting
You are talking about lines 146 and 152 in search.scss, I believe. The reason for this is that in FacetList.phtml in line 14 we make sure that the field name is added as a class to the
So, we get divs with As it is, we are already singling out Therefore, I see no reason why we should not refer to
I'll see to that! Cheers! P.S. For some reason I fail to find out how to insert quotes from the files directly here in the code, like you do. |
afde363 to
79ceff0
Compare
My goal, as a follow-up to this PR, is to add a config setting to facets.ini that replaces the hard-coded callnumber-first reference in FacetList.phtml, so that ANY field can be configured to show up as double-rows. A config setting can influence a template, but it cannot dynamically generate SCSS. Therefore, I'm looking for a more generic mechanism, so that if somebody wants to customize the column behavior, they only need to change one thing and not two -- that's why I'm suggesting a more semantic CSS class name, and a more general approach to the implementation of styles. I still think it is valuable to include field-name-specific classes on things to support very targeted local customizations, but I think it is better not to use such specific classes in default styles, because it makes customization more difficult.
If you go to the "Files changed" tab and click on a line number, then shift-click on another line number, it will select a set of lines. You can then click the little + button to add a comment on those lines. When you do this, there is also a "suggest changes" button in the text editor that can be used to directly propose changes to the code. A bit of a hidden feature, but very useful! |
demiankatz
left a comment
There was a problem hiding this comment.
I'm putting this back into "request changes" mode until we finish our conversation about the CSS class strategy and configurability. Once we are on the same page about that, I'll do another hands-on review, but I don't want to look again until I'm certain things won't change another time. :-)
Ok, that was the piece of information I was missing. Yes of course that makes perfect sense. |
The |
|
Hi Demian, |
demiankatz
left a comment
There was a problem hiding this comment.
Thanks, @ckaz, this does indeed look like what I had in mind; there are just a few details to adjust (see below).
Also, I notice there is a merge conflict here that can likely be resolved by merging the dev branch, rebuilding the CSS, and committing the resulting files to clean up the conflict. If you need any help sorting that out, let me know and I'd be happy to take care of it for you.
I haven't done hands-on testing here yet, but I will once this review has been addressed (or if you need my help with any additional parts of it).
| $maxListLength = $field == 'callnumber-first' | ||
| ? $columnSize * 2 : $columnSize; | ||
| // Two-column facets (configured in facets.ini: [HomePage_Settings] two_column_facets): | ||
| //$useTwoColumns = in_array($field, $twoColumnFacets ?? [], true); |
There was a problem hiding this comment.
Is this supposed to be commented out? I suspect not:
| //$useTwoColumns = in_array($field, $twoColumnFacets ?? [], true); | |
| $useTwoColumns = in_array($field, $twoColumnFacets ?? [], true); |
| language = Language | ||
| format = Format | ||
| ;hierarchy_top_title = Collections | ||
| hierarchy_top_title = Collections |
There was a problem hiding this comment.
I think this was uncommented by accident:
| hierarchy_top_title = Collections | |
| ;hierarchy_top_title = Collections |
| 'hierarchicalFacets' => $this->getHierarchicalFacets($facetConfig), | ||
| 'hierarchicalFacetSortOptions' => | ||
| $this->getHierarchicalFacetSortSettings($facetConfig), | ||
| 'twoColumnFacets' => $this->getTwoColumnFacets($facetConfig), |
There was a problem hiding this comment.
I believe that you can simplify this by adding:
use VuFind\Config\Feature\ExplodeSettingTrait;
at the top of the class definition, and then changing this line to:
| 'twoColumnFacets' => $this->getTwoColumnFacets($facetConfig), | |
| 'twoColumnFacets' => $this->explodeListSetting($facetConfig->HomePage_Settings->two_column_facets ?? ''), |
This existing trait does much the same thing as your getTwoColumnFacets method, so we might as well use the standard logic instead of creating a new variation.


Replaces #4231.