Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.x] Refactor navigation menu container bindings #1581

Conversation

caendesilva
Copy link
Member

Targets v2.x via #1568

See #1568 (review)

Copy link

codecov bot commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7f9b0c6) 100.00% compared to head (805849b) 99.91%.
Report is 1 commits behind head on improved-navigation-internals.

❗ Current head 805849b differs from pull request most recent head f19e288. Consider uploading reports for the commit f19e288 to get more accurate results

Additional details and impacted files
@@                         Coverage Diff                         @@
##             improved-navigation-internals    #1581      +/-   ##
===================================================================
- Coverage                           100.00%   99.91%   -0.09%     
+ Complexity                            1758     1757       -1     
===================================================================
  Files                                  183      183              
  Lines                                 4756     4754       -2     
===================================================================
- Hits                                  4756     4750       -6     
- Misses                                   0        4       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@caendesilva caendesilva force-pushed the refactor-navigation-menu-container-bindings branch from ca7e73c to c9414b5 Compare February 22, 2024 15:29
Revert "Extract fluent testing helper"

This reverts commit 1a8112e.

Revert "Boot kernel for test"

This reverts commit 06f0db4.

Revert "Boot kernel for unit test"

This reverts commit 5f9a8fc.

Revert "Boot the kernel for each test"

This reverts commit 02296ab.
@caendesilva caendesilva force-pushed the refactor-navigation-menu-container-bindings branch from 34a055d to 63a2d87 Compare February 22, 2024 16:06
This reverts commit a746597.
@caendesilva caendesilva force-pushed the refactor-navigation-menu-container-bindings branch from 63a2d87 to ff17137 Compare February 22, 2024 16:06
@caendesilva caendesilva force-pushed the refactor-navigation-menu-container-bindings branch from 7248ccd to a25423e Compare February 22, 2024 17:01
Being able to do app(MainNavigationMenu::class) instead of MainNavigationMenu::get() offers little value. It does have some benefit compared to using the aliases as the former can probably get better IDE support, but since the idea behind the alias is convenience I don't think it's worth it since the static method offers full type support. We also don't need the initial null bindings as the string versions throw an exception instead of trying to resolve the classes. It does mean the static get method breaks but that's fine as I will revert that next
@caendesilva caendesilva force-pushed the refactor-navigation-menu-container-bindings branch from e414a8b to 1ec568e Compare February 22, 2024 17:23
@caendesilva caendesilva force-pushed the refactor-navigation-menu-container-bindings branch from 1ec568e to fce1f3f Compare February 22, 2024 17:25
@caendesilva caendesilva force-pushed the refactor-navigation-menu-container-bindings branch from 77d6a42 to 5afae61 Compare February 22, 2024 17:27
@caendesilva caendesilva added the WTD Calls What The Diff label Feb 22, 2024
Copy link

what-the-diff bot commented Feb 22, 2024

PR Summary

  • Update Navigation Methods in Sidebar and Main Menu View Files
    The files 'sidebar.blade.php' and 'navigation.blade.php' have been updated to use simplified methods for accessing main and sidebar navigation menus, improving maintainability of the code.

  • Register Navigation Menus via Singleton Method
    In the 'NavigationServiceProvider.php' file, both main and sidebar navigation menus are now registered using the singleton method, enhancing the efficiency of menu generation.

  • Enhancements to Navigation Menu Retrieval
    The 'DocumentationSidebar.php' and 'MainNavigationMenu.php' files are now equipped with a get() method, allowing efficient retrieval of the navigation menu instance from the service container. These changes enhance the flexibility and ease of menu navigation.

  • Deletion of Redundant Navigation Manager
    The 'NavigationManager.php' file has been removed, reducing code clutter.

  • Addition of New Tests and Enhancements to Existing Tests
    Several test files ('HydeServiceProviderTest.php', 'AutomaticNavigationConfigurationsTest.php', 'DarkmodeFeatureTest.php', 'NavigationMenuTest.php', and 'Services/DocumentationSidebarTest.php') have been updated or expanded, including addition of Hyde::boot() calls and increased coverage for the NavigationMenu class. These changes aim to enhance the reliability and robustness of our codebase.

  • Removal of Redundant Comment
    A superfluous comment was removed from 'NavigationMenu.php', enhancing code cleanliness.

  • Additional Test Coverage
    'NavigationMenuUnitTest.php' and 'NavigationMenuViewTest.php' files have been updated with additional test coverage further ensuring proper functionality of the navigation menus.

@caendesilva caendesilva marked this pull request as ready for review February 22, 2024 17:43
@caendesilva caendesilva merged commit 70cff67 into improved-navigation-internals Feb 22, 2024
6 checks passed
@caendesilva caendesilva deleted the refactor-navigation-menu-container-bindings branch February 22, 2024 17:44
@caendesilva caendesilva changed the title [2.x] Refactor navigation menu container bindings [2.x] Refactor navigation menu container bindings Mar 22, 2024
@caendesilva caendesilva mentioned this pull request Jun 27, 2024
74 tasks
@caendesilva caendesilva removed the WTD Calls What The Diff label Jun 30, 2024
@caendesilva caendesilva added this to the v2 milestone Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant