Conversation
Co-authored-by: samdark <47294+samdark@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #302 +/- ##
=============================================
- Coverage 100.00% 99.96% -0.04%
- Complexity 705 711 +6
=============================================
Files 22 22
Lines 2922 2934 +12
=============================================
+ Hits 2922 2933 +11
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Nav::activateParents() that makes parent dropdown active when one of its child items is active
There was a problem hiding this comment.
Pull request overview
Re-introduces Nav::activateParents() to optionally mark a dropdown toggler as active when one of its child dropdown items is active.
Changes:
- Add
Nav::activateParents()API and internal logic to applyactiveto dropdown togglers based on active children. - Add/extend PHPUnit coverage for the new behavior and immutability.
- Document the enhancement in the changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/NavTest.php | Adds tests validating parent dropdown activation behavior and immutability for activateParents(). |
| src/Nav.php | Implements activateParents flag, helper to detect active dropdown children, and applies active class to the dropdown toggler. |
| CHANGELOG.md | Adds an entry documenting the new Nav::activateParents() enhancement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Whether to activate the parent dropdown item when one of its child items is active. | ||
| * | ||
| * When set to `true`, if a dropdown item's URL matches {@see currentPath}, the dropdown toggle (parent) will also | ||
| * receive the `active` CSS class. | ||
| * | ||
| * @param bool $enabled Whether to activate parent items. Defaults to `false`. | ||
| * | ||
| * @return self A new instance with the specified activate parents value. | ||
| * | ||
| * Example usage: | ||
| * ```php |
There was a problem hiding this comment.
The PHPDoc states “Defaults to false” for $enabled, but the method signature doesn’t provide a default value. To avoid confusing API consumers, either remove the “Defaults…” wording (and rely on the property default) or change the signature to include an explicit default (if that’s intended).
| $dropDownItems = $this->isDropdownActive($items); | ||
| $togglerClasses = ['nav-link', 'dropdown-toggle']; | ||
|
|
||
| if ($this->activateParents && $this->hasActiveDropdownItem($dropDownItems)) { | ||
| $togglerClasses[] = self::NAV_LINK_ACTIVE_CLASS; | ||
| } |
There was a problem hiding this comment.
$dropDownItems is a Dropdown instance (returned by isDropdownActive()), but the name reads like an array of items and uses inconsistent casing (“dropDown” vs “dropdown”). Consider renaming to something like $dropdown / $dropdownWithActiveItems for clarity and consistency.
Thanks for assigning this issue to me. I'm starting to work on it and will keep this PR's description up to date as I form a plan and make progress.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.