Repository navigation
Conversation
…user interface change
nkissebe
left a comment
There was a problem hiding this comment.
Thanks. The one behavioural fix here is real: a synthesised category with no path produces a sectionless link, the router drops the empty section=, and /kb?sort=... lands on the wrong task so sorting on the home page is ignored. A few things before this can go in:
The UI change isn't in the PR. displayTask now builds a category and hands it to the view, but nothing in the PR reads it: core views/articles/tmpl/display.php is untouched and never references $this->category, and the two new language keys are referenced nowhere in core/. The sidebar/contact markup the description talks about must live in an uncommitted app/templates/doi override. Merged as-is, every hub gets a controller change with no consumer and two orphan strings, and the actual change is unreviewable. Please include the display.php change in core alongside this (template overrides are for CSS/skin, structural HTML belongs in core).
Fix the model, not each call site. Rather than remembering set('path', 'all') everywhere a Category is synthesised, give Category::link() a fallback when path is empty ($this->get('path') ?: $this->get('alias')). That fixes both existing call sites and every future one.
Extract the synthetic "all" category. With this PR the six-line construction is copy-pasted verbatim in displayTask and categoryTask. The bug being fixed (one copy missing a field) is exactly what that duplication produces. A private allCategory() on the controller, or a static factory on the model, called from both, removes the drift.
Inline notes below.
| $category->set('published', 1); | ||
| $category->set('path', 'all'); | ||
|
|
||
| $this->view->set('category', $category); |
There was a problem hiding this comment.
Besides the duplication noted above:
- Lines 50, 57 and 59 pick up trailing tabs (
git diff --checkflags them; the rest of the file has none). - The standalone
$this->view->set('category', $category)sits right above the existing fluent->set('archive')->setLayout()->display()chain; fold it into the chain. - If the display layout is going to adopt the category sidebar from
category.php, note that markup reads$this->catid(active-state highlighting) and$this->filters['search'], neither of whichdisplayTasksets. Either setcatid = 0and afiltersarray here too, or move the sidebar into a shared partial that takes explicit arguments.
| COM_KB_POPULAR_ARTICLES="Most Popular Articles" | ||
| COM_KB_RECENT_ARTICLES="Most Recent Articles" | ||
| COM_KB_CONTACT_US="Contact Us" | ||
| COM_KB_CONTACT_INFORMATION="Contact Information" |
There was a problem hiding this comment.
Nothing in core/components/com_kb references COM_KB_CONTACT_US or COM_KB_CONTACT_INFORMATION. If these serve the doi template override, they belong in that template's language file (or app/bootstrap/site/language/overrides/en-GB.override.ini); if they serve a core display.php change, that change needs to be in this PR.
There was a problem hiding this comment.
Sorry that I need mention that the override view changes are in PR: https://gitlab.hubzero.org/hub-purr/tpl_doi/-/merge_requests/18.
nkissebe
left a comment
There was a problem hiding this comment.
Second pass, after finding the views this change serves: they are on tpl_doi branch kuang_local_branch_doi_template_10022026 (commit 873ff78), not deployed on purr or dev yet.
The split is the wrong way round. This PR puts a controller tweak and two strings into core while the real change sits in a full-copy template override. Reading that override: the search box in the page header, the category sidebar on the home page, sort and category links that keep the search term, and highlighted snippets in search results are all generic. Nothing in them is PURR-specific, and as a 300-line copy of a core view the override had already lost the April accessibility fixes (translated nav label, aria-current). The only PURR-specific pieces are the contact-page link in place of the Answers/Wishlist/Support blocks and the sidebar-on-the-left layout.
Also, the home view never needed the controller change. The new override touches $this->category only inside an isset guard that is never true on the home page, and hardcodes its sort links to section=all. What it does need is the path fix, and that is core's own bug: the synthetic "all" category has no path, so even core's /kb/all sort links carry an empty section and route to the wrong task. That belongs in Category::link() as a fallback to the alias, not at each call site. The two COM_KB_CONTACT_* strings are used only by the override, so they belong in the template's language file.
I've done it that way:
- #1939 supersedes this PR (your commit carried, rework on top):
_search,_categoriesand_helppartials,Article::snippet()with tests, the link fallback, oneallCategory()helper, and the home view given the same contract as the category view. tpl_doibranchkb-core-views(pushed to GitLab) dropsdisplay.php/category.phpand their.origsnapshots, and keeps_help.php, akb.cssoverride that imports core's rules and floats the sidebar left, and the two strings. A merge request still needs opening there: https://gitlab.hubzero.org/hub-purr/tpl_doi/-/merge_requests/new?merge_request%5Bsource_branch%5D=kb-core-views
Deploy the two together; the template branch on its own would expose core's current views. One more thing noticed on the way: dev.purr's working copy of articles.php is an older file with the recent comment-ownership and sanitising checks missing, with this change layered on top. Worth refreshing that checkout before testing there.
|
Merged via #1939: your commit carried as-is on top of current 2.4-main, plus the rework described in the reviews above (search in the page header, shared category and help partials, highlighted search snippets with tests, the category link fallback, and the contact strings moved to the template). The template side is on tpl_doi branch kb-core-views on GitLab; deploy the two together. |
PURR Ticket: https://purr.purdue.edu/support/ticket/2814
We wanted to update the user interface of the knowledge base home page and the searching result page to include:
On knowledge base home view (such as https://purr.purdue.edu/kb), display the Categories list on the left pane for navigation, and display the articles on the right pane. Same applied on category article view (such as https://purr.purdue.edu/kb/aboutpurr?sort=recent) and searching result view.
When a searching for articles is done, display the matched articles by its category. Under the article title, display the excerpt from the article that includes the searching term. This helps user quickly locate the article that is wanted.
Issues found and fixed in com_kb\site\controllers\articles.php:
path of Category model instance $category is not set, which causes the section parameter is not set in the $category method link() and the sort link won't work as expected.
Test steps:
Test is done on dev PURR and it works as expected.