BUG Don't double up the breadcrumb on list views #2976
Conversation
maxime-rainville
force-pushed
the
pulls/5.2/list-view-breadcrumb
branch
from
37f25e2 to
87a3043
Compare
There was a problem hiding this comment.
While I appreciate the initiative with all of the refactoring, it made it a lot more time consuming to review since I needed to compare the original code with the new code and parse out the changes and see what moved where, etc etc.
In the future can you please try to keep refactoring separate from fixing the bug? At the very least having separate commit (one dedicated to just what's necessary to fix the bug, and a separate one for the refactoring) would make it a lot easier to keep track of.
| /** | ||
| * Push the provided an extra breadcrumb crumb at the end of the provided List | ||
| */ | ||
| private function pushCrumb(ArrayList $items, string $title, string|false $link): void | ||
| { | ||
| $items->push(ArrayData::create([ | ||
| 'Title' => $title, | ||
| 'Link' => $link | ||
| ])); | ||
| } |
There was a problem hiding this comment.
This extra method seems kinda unnecessary but not worth the ping pong to remove it, since it's private anyway.
|
Please also change the commit prefix to |
maxime-rainville
force-pushed
the
pulls/5.2/list-view-breadcrumb
branch
from
b1a8601 to
9072345
Compare
There was a problem hiding this comment.
While I appreciate the initiative with all of the refactoring, it made it a lot more time consuming to review since I needed to compare the original code with the new code and parse out the changes and see what moved where, etc etc.
In the future can you please try to keep refactoring separate from fixing the bug? At the very least having separate commit (one dedicated to just what's necessary to fix the bug, and a separate one for the refactoring) would make it a lot easier to keep track of.
Sorry for the unduly big change. But the more I looked at this thing, the more it seemed like the bugs were fundamentally caused by the bad implementation.
Refactoring the code while replicating the bugs would have probably made this PR even more complex. And I don't think there would have been a way to fix the bugs without addressing the underlying poor implementation.
Please also change the commit prefix to FIX as per the commit message guidelines linked in the PR description.
Done.
There was a problem hiding this comment.
LGTM, works well locally.
I found a new related bug that when I'm in list view and drill down to a specific page, if I then toggle to "tree view" it'll retain the ?ParentID=9 in the URL and keeps the breadcrumbs there. But I tested that without this PR and it was still happening so it's an existing bug.
Description
This PR refactors how the CMS breadcrumb is generated.
It fix a bug causing crumbs to be duplicated on the list view.
It also ensures the "Pages" top crumb is always shown on the list view.
It adds missing unit tests.
Manual testing steps
Issues
Pull request checklist