Search: Add breadcrumbs to search results in order to provide more context#2282
Open
Tschuppi81 wants to merge 63 commits intomasterfrom
Open
Search: Add breadcrumbs to search results in order to provide more context#2282Tschuppi81 wants to merge 63 commits intomasterfrom
Tschuppi81 wants to merge 63 commits intomasterfrom
Conversation
❌ 11 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Contributor
Author
|
Also i switched the red color in the results to primary and italic left: previous, right: now |
Contributor
Author
|
Not sure if the breadcrumbs color in search shall be |
There was a problem hiding this comment.
Pull request overview
This pull request adds breadcrumb navigation to search results for various content types (Topics, News, People, Files, Tickets, Events, Directory Entries, etc.) to provide better context when viewing search results. The implementation includes:
Changes:
- Renamed
PageLayouttoTopicLayoutthroughout the codebase for clarity - Added a new
get_layout()method to the request object to retrieve layouts for model instances - Implemented a layout registry system via a new
Layoutdirective - Updated search result templates to display breadcrumbs for various content types
- Added breadcrumb styling and fixed CSS issues related to z-index stacking
- Added file ID anchors and highlight styling for targeted files
- Updated test selectors to use regex anchors for more precise matching
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/onegov/winterthur/test_search.py | Updated click selectors to use regex anchors for precise matching |
| tests/onegov/town6/test_views_directory.py | Updated click selectors to use regex anchors for precise matching |
| tests/onegov/org/test_views_ticket.py | Updated click selectors to use regex anchors for precise matching |
| tests/onegov/org/test_views_directory.py | Updated click selectors to use regex anchors for precise matching |
| tests/onegov/org/test_layout.py | Updated imports from PageLayout to TopicLayout |
| src/onegov/town6/theme/styles/town6.scss | Added z-index reset for breadcrumb links |
| src/onegov/town6/theme/styles/search.scss | Added breadcrumb styling and updated search preview styles |
| src/onegov/town6/theme/styles/files.scss | Added highlight and scroll margin for targeted file rows |
| src/onegov/town6/templates/macros.pt | Added breadcrumbs to search result macros, changed p to div tags for consistency |
| src/onegov/town6/layout.py | Renamed PageLayout to TopicLayout, added layout decorators for multiple models, implemented breadcrumbs for GeneralFile and various RIS models |
| src/onegov/org/views/page.py | Updated imports and references from PageLayout to TopicLayout |
| src/onegov/org/views/editor.py | Updated imports and references from PageLayout to TopicLayout |
| src/onegov/org/request.py | Added get_layout() method to retrieve layouts for model instances |
| src/onegov/org/layout.py | Renamed PageLayout to TopicLayout, added FormDefinitionLayout, DirectoryLayout, improved breadcrumbs for various layouts |
| src/onegov/org/exports/base.py | Fixed import to use onegov.org instead of onegov.town6 |
| src/onegov/org/directives.py | Added Layout directive for registering layouts to models |
| src/onegov/org/app.py | Added layout directive to OrgApp |
| src/onegov/agency/views/page.py | Updated imports and references from PageLayout to TopicLayout |
| src/onegov/agency/layout.py | Updated imports and class name from PageLayout to TopicLayout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
BreathingFlesh
requested changes
Jan 13, 2026
Contributor
BreathingFlesh
left a comment
BreathingFlesh
left a comment
There was a problem hiding this comment.
I would give the breadcrumbs a light background so they look more like a separate element
Tschuppi81
commented
Feb 2, 2026
Daverball
reviewed
Feb 2, 2026
Member
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
I think there was some confusion, request is not a predicate it's just a regular parameter on get_layout, so you get back a layout instance for the given obj and request, you don't get a layout class back anymore.
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
Daverball
reviewed
Feb 3, 2026
Member
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
Looks like I forgot, that the first parameter to the dispatch_method for the app instance is required for all implementations, we still want the request parameter though, just not as a predicate and we want to return a layout instance, not a class.
Daverball
reviewed
Feb 24, 2026
Member
Daverball
left a comment
Daverball
left a comment
There was a problem hiding this comment.
We're getting pretty close, but the request parameter is still missing and some of the type annotations aren't quite correct.
You also made some changes to the breadcrumb links I'm not sure are a good idea, I'd rather you make that transformation when rendering the search result, since # should generally be able to be replaced by request.link(result). If that isn't the case for some models, I think I'd prefer if you added a new self_url property to Layout, which defaults to return self.request.link(self.model) and can be changed for any layouts where it's different.
Comment on lines
+426
to
+429
| except RegistrationError as e: | ||
| # ignore `already have registration for key` error | ||
| if 'Already have registration for key' not in str(e): | ||
| raise |
Member
There was a problem hiding this comment.
This is very suspect to me. The View action does not need to do a song and dance like this, so I'm pretty sure registering a different layout for the same predicate is fine, as long it's not on the same app class, i.e. registering for Page on OrgApp and then again for TownApp is fine, but registering Page twice on OrgApp isn't.
We may need to set the filter_convert and filter_compare attributes on the action to, so the perform calls happen correctly:
filter_convert = {
"model": dectate.convert_dotted_name
}
filter_compare = {
"model": morepath.directive.isbaseclass,
}You may also need to change self.identifier to {'model': self.model} or just self.model, but I'm not sure about that one.
| Framework.get_layout, | ||
| name='model', | ||
| default=None, | ||
| index=ClassIndex # type: ignore[arg-type] |
Member
There was a problem hiding this comment.
This is probably my mistake, this type: ignore can go away if you change PredicateAction.__init__ stubs/morepath/directive.pyi, the index parameter should be type[KeyIndex] | str instead of KeyIndex | str.
Comment on lines
+1819
to
+1820
| Link(self.model.number, self.request.link( | ||
| TicketCollection(self.request.session).by_id(self.model.id))) |
Member
There was a problem hiding this comment.
Did you change this for the search results, so the final breadcrumb points somewhere? I think I would prefer if you detected breadcrumb links with href # and replaced them with request.link(model) in the search view, not in the original layout. I'm also not sure why you're querying for the ticket, when you already have it as self.model, you should just do self.request.link(self).
I think in general we want the breadcrumb link to remain #, since it should generally point to where we currently are. The search view is the exception, so the search view can deal with that. So please revert any changes from # to an absolute link.
Comment on lines
+294
to
+297
| layout_class = self.app.get_layout(model) | ||
| assert layout_class is not None | ||
|
|
||
| return layout_class(model, self) |
Member
There was a problem hiding this comment.
Suggested change
| layout_class = self.app.get_layout(model) | |
| assert layout_class is not None | |
| return layout_class(model, self) | |
| layout= self.app.get_layout(model, self) | |
| assert layout is not None | |
| return layout |
This should now become just this with the new signature.
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
Co-authored-by: David Salvisberg <david.salvisberg@seantis.ch>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Search: Add breadcrumbs to search results in order to provide more context
TYPE: Feature
LINK: ogc-2880