Search: Add breadcrumbs to search results in order to provide more context#2282
Search: Add breadcrumbs to search results in order to provide more context#2282Tschuppi81 wants to merge 66 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files
... and 90 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
|
Also i switched the red color in the results to primary and italic left: previous, right: now |
|
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
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
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 <[email protected]>
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
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.
| except RegistrationError as e: | ||
| # ignore `already have registration for key` error | ||
| if 'Already have registration for key' not in str(e): | ||
| raise |
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.
| Link(self.model.number, self.request.link( | ||
| TicketCollection(self.request.session).by_id(self.model.id))) |
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.
Co-authored-by: David Salvisberg <[email protected]>
Co-authored-by: David Salvisberg <[email protected]>
Co-authored-by: David Salvisberg <[email protected]>
Co-authored-by: David Salvisberg <[email protected]>
Co-authored-by: David Salvisberg <[email protected]>
Co-authored-by: David Salvisberg <[email protected]>
Search: Add breadcrumbs to search results in order to provide more context
TYPE: Feature
LINK: ogc-2880