SF-2441 Focus draft tab on draft nav

[siltomato]

• Editor looks for URL query param draft-active=true to focus the draft tab.
• Modified updateAutoDraftTabVisibility() to only add a draft tab to 'source' if one isn't already in the tab state for either 'source' or 'target'.
• Removes 'draft-active' query string from url when another tab from 'source' is selected.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 77.53%. Comparing base (2655dcb) to head (890685b).

Files	Patch %	Lines
...shared/sf-tab-group/tab-state/tab-state.service.ts	80.00%	1 Missing and 1 partial ⚠️
...ntApp/src/app/translate/editor/editor.component.ts	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2486      +/-   ##
==========================================
- Coverage   77.53%   77.53%   -0.01%     
==========================================
  Files         511      511              
  Lines       29260    29272      +12     
  Branches     4753     4757       +4     
==========================================
+ Hits        22687    22696       +9     
- Misses       5829     5831       +2     
- Partials      744      745       +1     

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

[Nateowami]

This is working great; thanks. Are there still changes coming that make it a draft PR?

[Nateowami]

Reviewed 2 of 4 files at r1, 1 of 2 files at r2.
Reviewable status: 2 of 5 files reviewed, 1 unresolved discussion (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1332 at r2 (raw file):

          .subscribe(() => {
            this.router.navigate([], {
              queryParams: { 'draft-active': null },

Good catch. I was about to comment this when I saw you had updated the PR. However, I tested this locally before you made the update, and it worked. It seems like it shouldn't have...

Also, if you wanted to keep paramKey as a variable, you should be able to do that with queryParams: { [paramKey]: null },

[siltomato]

I added the query param to router link in DraftPreviewBooksComponent. Is there another place it needs to be added?

Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.ts line 1332 at r2 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

Good catch. I was about to comment this when I saw you had updated the PR. However, I tested this locally before you made the update, and it worked. It seems like it shouldn't have...

Also, if you wanted to keep paramKey as a variable, you should be able to do that with queryParams: { [paramKey]: null },

Ah, cool, nice one. I'll have to remember that!

[Nateowami]

I added the query param to router link in DraftPreviewBooksComponent. Is there another place it needs to be added?

No; that's the only place, at least for now.

Reviewed 1 of 4 files at r1, 1 of 2 files at r2, all commit messages.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts line 3826 at r2 (raw file):

      env.component.tabState.tabs$.pipe(take(1)).subscribe(tabs => {
        expect(tabs.filter(tab => tab.type === 'draft')[0]?.isSelected).toBe(true);

Filtering an array and taking the first result is the same as Array.find. This could just be:

expect(tabs.find(tab => tab.type === 'draft')?.isSelected).toBe(true);

(Same below)

Also, having expectations only in a subscription makes it difficult to be certain your expectations actually run.

[Nateowami]

I've commented some things that could be improved, but I'd be happy to merge as-is after I do a bit more testing.

Reviewed 1 of 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @siltomato)

[Nateowami]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

[siltomato]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts line 3826 at r2 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

Filtering an array and taking the first result is the same as Array.find. This could just be:

expect(tabs.find(tab => tab.type === 'draft')?.isSelected).toBe(true);

(Same below)

Also, having expectations only in a subscription makes it difficult to be certain your expectations actually run.

Good catch. Fixed. What's the best way to not run in the subscription?

[Nateowami]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts line 3826 at r2 (raw file):

Previously, siltomato wrote…

Good catch. Fixed. What's the best way to not run in the subscription?

One option would be awaiting a promise. But it's not critical. Do you want to just go ahead and merge it? It can be tested after merging. This is the final piece of the puzzle for next week's release.

[siltomato]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts line 3826 at r2 (raw file):

Previously, Nateowami (Nathaniel Paulus) wrote…

One option would be awaiting a promise. But it's not critical. Do you want to just go ahead and merge it? It can be tested after merging. This is the final piece of the puzzle for next week's release.

Sure.