Settings nav

[DmitiryPotychkin]

Add parameter skipLocationChange for not add certain location to location history stack.

1. ht-navigation-list receive list of links, link may have parameter skipLocationChange
2. Value of this parameter with other arguments passed to navigate service method navigateWithinApp and after it to navigate method
3. if value of skipLocationChange is true than service Router will receive skipLocationChange: true in extras, and this location will not be added to location history stack after routing

[codecov]

Codecov Report

Merging #534 (1e8fd7f) into main (3b1f8be) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #534   +/-   ##
=======================================
  Coverage   85.57%   85.57%           
=======================================
  Files         758      758           
  Lines       15525    15525           
  Branches     1839     1839           
=======================================
  Hits        13285    13285           
  Misses       2209     2209           
  Partials       31       31           

Impacted Files	Coverage Δ
...onents/src/navigation/navigation-list.component.ts	85.18% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b1f8be...1e8fd7f. Read the comment docs.

[aaron-steinfeld]

this is a shortcut method, instead of modifying this with more optional parameters, can you use navigate instead?

[DmitiryPotychkin]

What will be better - create new method similar to navigateWithinApp in navigation service (maybe it'll be helpful in future or it's a rare case?) or just directly use navigate method inside navigation-list component without any shortcut?

[aaron-steinfeld]

I would leave navigationService untouched, and call via:

  this.navigationService.navigate({
      navType: NavigationParamsType.InApp,
      path: item.matchPaths[0],
      replaceCurrentHistory: item.skipLocationChange,
      relativeTo: this.activatedRoute
  });

Replacing the history, when changing something other than a query param (which is already supported) should be a rare case. I'm not convinced we need it here, but I definitely don't want to encourage it for future use cases.

[aaron-steinfeld]

(you beat me to this with the exact change 😄 )

[aaron-steinfeld]

this is already captured on line 306 (replaceCurrentHistory)

[DmitiryPotychkin]

Got it, thanks, will use it instead adding new

[aaron-steinfeld]

Commented above

[aaron-steinfeld]

So missed this the first time around, but we should clarify our naming (the test actually caught this). There's two separate booleans that the angular supports:

• skipLocationChange: When true, navigates without pushing a new state into history.
• replaceUrl: When true, navigates while replacing the current state in history.

To illustrate the distinction, imagine three nav items that are navigated in order (first -> second -> third)

first: { ...,  skipLocationChange: false},
second: { ...,  skipLocationChange: true},
third: { ...,  skipLocationChange: false}

with the behavior of skipLocationChange, when complete our history stack looks like:

<third> // Active
<first> // browser back

with the behavior of replaceUrl, when complete our history stack looks like:

<third> // Active
<second> // browser back

A subtle distinction, but our implementation uses replaceUrl, so we should name the item's property accordingly.

[aaron-steinfeld]

@DmitiryPotychkin can we update the naming here to reflect the behavior? In other words, skipLocationChange on line 124 should be named replaceCurrentHistory.

[DmitiryPotychkin]

@aaron-steinfeld thank you for the explanation!

[github-actions]

Unit Test Results

    4 files  ±0  232 suites  ±0   14m 16s ⏱️ ±0s
820 tests +2  820 ✔️ +2  0 💤 ±0  0 ❌ ±0 
824 runs  +2  824 ✔️ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 7ad445c. ± Comparison against base commit 3b1f8be.