SF-3028 Add final confirmation step when generating drafts

[josephmyers]

I believe this may actually be the first of the mixed source UI PR's to be ready.

---

This change is 

[Nateowami]

Here's a screenshot of about where we were at the end of the conversation, for reference.

[josephmyers]

Two things Bethany suggested that I think are worth capturing here:

• Using ranges when training books have the exact same source/target
• Collapsing the training card by default

[josephmyers]

Given how condensed the training card can be with the latest changes, I think we should postpone the expand/collapse card til we confirm we need it.

[Nateowami]

This is an improvement, but:

• I think there should be 4 rows here, not one giant cell
• List of books to translate is still not in the same style as the books to draft from. Preferably same font size, comma separated. That's what I recall discussing last time we met.
• Book names should be localized using the I18nService
• I also think the font weight of 100 is too light.

[josephmyers]

• I think there should be 4 rows here, not one giant cell

I am surprised to hear you say this. This would be going back toward the more blatant table style Bethany criticized last week. Combining the identical rows provides much-needed spacing. This is what the table style looks like:

• List of books to translate is still not in the same style as the books to draft from. Preferably same font size, comma separated. That's what I recall discussing last time we met.

I'm not understanding. Aren't the books to translate and the books to draft from the same thing?

• Book names should be localized using the I18nService

Yes, thanks. I haven't done localization yet (see raw text in HTML), but I'll do this one now. Should I fix the other places in the component that are forcing English, too?

• I also think the font weight of 100 is too light.

I disagree. The lighter weight breaks up the monotony of the text and allows us to focus on the book data without needing to increase its weight. This weight is particularly suited to explanatory text like this, which isn't intended to be the focus, while still being readable. Unless you think it's not readable? I'm open to other suggestions, but simply using the standard font weight looked bad.

I do notice that 100-300 are all the same. Maybe we could define a level in between what we have now and the standard?

[josephmyers]

Talked with Nathaniel. It appears that the font sizes between the top and bottom are slightly different. I'll look into this. And we do in fact want the bottom books comma separated on one line.

[josephmyers]

At the recent UX meeting, we decided to combine the identical training book ranges to a single cell, comma separated, with word/span wrapping that doesn't separate the individual ranges.

Edit: with full book names

[Nateowami]

After meeting with Nigel, this is looking good. Several things:

• Remove margin-bottom from .explanation
• Localize everything, including book names
• Instead of list.join(', '), add this method to the I18nService and use it instead (not all languages do commas the same as English):

  enumerateList(list: string[]): string {
    return new (Intl as any).ListFormat(this.localeCode, { style: 'long', type: 'conjunction' }).format(list) as string;
  }

I think we're both happy with it once these changes are included.

[Nateowami]

This should be updated to use i18n.enumerateList

[josephmyers]

Done. We do lose out on the word-wrapping control we had previously, however

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.93%. Comparing base (f54210b) to head (84c4c06).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2787      +/-   ##
==========================================
+ Coverage   80.91%   80.93%   +0.02%     
==========================================
  Files         533      533              
  Lines       31224    31253      +29     
  Branches     5085     5090       +5     
==========================================
+ Hits        25265    25296      +31     
- Misses       5198     5207       +9     
+ Partials      761      750      -11     

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

[josephmyers]

@Nateowami, it sounds like you're owning the review on this? Or maybe @nigel-wells?

[Nateowami]

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html line 204 at r3 (raw file):

              <td mat-cell *matCellDef="let element" class="bookName">
                <div class="confirm-books cell-padding-block">
                  {{ this.i18n.enumerateList(element.ranges) }}

this. is not needed, here or below.

[josephmyers]

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html line 204 at r3 (raw file):

Previously, Nateowami wrote…

this. is not needed, here or below.

Done

[nigel-wells]

Happy to take a look if needed - @Nateowami has assigned himself though so I've not taken a deep look other than what we spoke about yesterday.

[Nateowami]

I've been trying to test this out locally, but it's completely non-functional. I think it's a combination of a bug in the training step (see SF-3141), and this component not reading the training step properly (which is understandable; it wasn't originally written against the same logic).

If I make this selection:

I end up with this clearly incorrect summary: