Skip to content

Conversation

StephDriver
Copy link
Contributor

closes #4839

@StephDriver
Copy link
Contributor Author

A11y Syntax Notes

OLH theme has space around sections, so when they are put in to make landmarks, this can cause layout shifts. So now we have a class for when these are being used 'invisibly' to provide landmarks.

section.invisible-landmark{
  padding: 0;
  margin: 0;
}

Currently this is only a concern on OLH, as the other two themese don't ahve spacing, however it may be useful to extend this to all three themes and markup any sections that are purely to provide aria landmarks with this class.

Copy link
Member

@joemull joemull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Found a few small bugs, and also added a few comments about readability around keeping this code distinct from the templates and patterns that belong to GenericFacetedListView, which granted are not very well documented as belonging specifically to it!

@StephDriver StephDriver requested a review from joemull September 23, 2025 08:38
@StephDriver StephDriver assigned joemull and unassigned StephDriver Sep 23, 2025
@joemull joemull requested a review from mauromsl September 29, 2025 09:21
@joemull joemull assigned mauromsl and unassigned joemull Sep 29, 2025
@StephDriver StephDriver mentioned this pull request Oct 3, 2025
Copy link
Member

@mauromsl mauromsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, just one comment regarding the value "all" for the pagination queryparameter


paginate_by = request.GET.get("paginate_by", 25)
if paginate_by == "all":
paginate_by = len(articles) if articles else 25
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of implementation, qs.count() is always preferred for querysets over len(qs)

I wonder, however, if we really want to support "all" as a feature, since we do not have a dynamic loader for this, we could put a lot of strain on the server. I don't think the user will be happy waiting for potentially thousands of results to be returned and displayed all at once

@mauromsl mauromsl assigned StephDriver and unassigned mauromsl Oct 16, 2025
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.

Full-text search results are not fully navigable

3 participants