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] Simplify navigation item group state handling to be deterministic #1632

Conversation

caendesilva
Copy link
Member

Targets #1568

Verified

This commit was signed with the committer’s verified signature.
caendesilva Caen De Silva

Verified

This commit was signed with the committer’s verified signature.
caendesilva Caen De Silva

Verified

This commit was signed with the committer’s verified signature.
caendesilva Caen De Silva

Verified

This commit was signed with the committer’s verified signature.
caendesilva Caen De Silva
Want to move away from depending so much on the keys
Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (2c64394) to head (63ca58a).

Additional details and impacted files
@@                         Coverage Diff                         @@
##             improved-navigation-internals    #1632      +/-   ##
===================================================================
- Coverage                            99.85%   99.85%   -0.01%     
- Complexity                            1782     1783       +1     
===================================================================
  Files                                  185      185              
  Lines                                 4814     4812       -2     
===================================================================
- Hits                                  4807     4805       -2     
  Misses                                   7        7              

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

@caendesilva
Copy link
Member Author

caendesilva commented Mar 22, 2024

Tests are failing because of how the unit tests are set up. Deterministically discovering which group is active means that the groups actually need to be present in the kernel, just mocking the current page is not enough. See dc4a539

@caendesilva caendesilva force-pushed the simplify-navigation-item-group-state-handling branch from d42416a to 7161f0b Compare March 22, 2024 17:05
@caendesilva caendesilva force-pushed the simplify-navigation-item-group-state-handling branch from 8a2e460 to 2f8b7cc Compare March 22, 2024 17:17
@caendesilva caendesilva force-pushed the simplify-navigation-item-group-state-handling branch from 10b9797 to 5b51089 Compare March 23, 2024 18:26
@caendesilva caendesilva force-pushed the simplify-navigation-item-group-state-handling branch from 5c9a917 to d0ef005 Compare March 23, 2024 18:44
Performance consideration is negligible (new is 0.038ms instead of 0.085ms) but it makes it more understandable and reduces the need of a complex loop
@caendesilva caendesilva force-pushed the simplify-navigation-item-group-state-handling branch from 24d7e2e to c860213 Compare March 23, 2024 18:52
@caendesilva caendesilva marked this pull request as ready for review March 23, 2024 19:03
@caendesilva caendesilva merged commit af2a511 into improved-navigation-internals Mar 23, 2024
8 checks passed
@caendesilva caendesilva deleted the simplify-navigation-item-group-state-handling branch March 23, 2024 19:07
@caendesilva caendesilva mentioned this pull request Jun 27, 2024
99 tasks
@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.

None yet

1 participant