SF-3528 Require users to select formatting options the first time

[RaymondLuong3]

The first time a user generates a draft or if they have never formatted their draft previously, show the user the UI and cause them to format their draft. See the balsamiq for details.

Desktop

Mobile

---

This change is 

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.09%. Comparing base (91d70de) to head (9f65e32).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3425   +/-   ##
=======================================
  Coverage   82.08%   82.09%           
=======================================
  Files         611      611           
  Lines       36314    36317    +3     
  Branches     5990     5985    -5     
=======================================
+ Hits        29807    29813    +6     
+ Misses       5642     5624   -18     
- Partials      865      880   +15     

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

[Nateowami]

Reviewable status: 0 of 5 files reviewed, 5 unresolved discussions

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.scss line 35 at r1 (raw file):

  align-items: center;

  ::ng-deep mat-icon {

The flex-shrink: 0 belongs on the button, not the icon in the button. That will avoid the need for ::ng-deep and make the container handle layout better.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html line 32 at r1 (raw file):

              <mat-icon>build</mat-icon>
              <div class="hide-lt-lg">{{ t("formatting_options") }}</div>
              <div class="hide-gt-lg">{{ t("options") }}</div>

If there's only one word that can be shown, I kind of think "formatting" makes more sense than just "options"

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html line 34 at r1 (raw file):

              <div class="hide-gt-lg">{{ t("options") }}</div>
            </button>
            <span>{{ t("new") }} {{ t("change_line_breaks_and_quotation_marks") }}</span>

The mockup calls for this message to be hidden after 60ish days of going live. Obviously we don't know exactly when this feature will go live, but in the past we've done this by adding a property e.g. showOptionsNewMessage = new Date() > new Date('2025-12-01') along with a comment. This helps prevent us from just forgetting to remove it in the future.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts line 41 at r1 (raw file):

const mockedFeatureFlagsService = mock(FeatureFlagService);

fdescribe('DraftHistoryEntryComponent', () => {

Missed fdescribe

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 275 at r1 (raw file):

  private _requireSelectingFormattingOptions?: boolean;
  get requireSelectingFormattingOptions(): boolean {
    return this._requireSelectingFormattingOptions ?? false;

This also need to be dependent on the feature flag.

[RaymondLuong3]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html line 32 at r1 (raw file):

Previously, Nateowami wrote…

If there's only one word that can be shown, I kind of think "formatting" makes more sense than just "options"

Good idea. Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html line 34 at r1 (raw file):

Previously, Nateowami wrote…

The mockup calls for this message to be hidden after 60ish days of going live. Obviously we don't know exactly when this feature will go live, but in the past we've done this by adding a property e.g. showOptionsNewMessage = new Date() > new Date('2025-12-01') along with a comment. This helps prevent us from just forgetting to remove it in the future.

Yes, I think Dec 1 is a good benchmark. I've made the updates. I left the date as a separate variable showSelectFormatNoticeExpireDate since the balsamiq has a third scenario that depends on the 60 day timeframe.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.scss line 35 at r1 (raw file):

Previously, Nateowami wrote…

The flex-shrink: 0 belongs on the button, not the icon in the button. That will avoid the need for ::ng-deep and make the container handle layout better.

Good suggestion. Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 275 at r1 (raw file):

Previously, Nateowami wrote…

This also need to be dependent on the feature flag.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.spec.ts line 41 at r1 (raw file):

Previously, Nateowami wrote…

Missed fdescribe

Done.

[Nateowami]

@Nateowami reviewed 1 of 6 files at r2.
Reviewable status: 1 of 6 files reviewed, 7 unresolved discussions

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html line 31 at r2 (raw file):

            <button mat-flat-button class="format-usfm" [routerLink]="['format']">
              <mat-icon>build</mat-icon>
              <div class="hide-lt-lg">{{ t("formatting_options") }}</div>

These should be spans, not divs. Neither has any semantic meaning, but spans are inline elements, and divs are block elements.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 273 at r2 (raw file):

  }

  private _requireSelectingFormattingOptions?: boolean;

This private property is causing more complexity than it is solving and isn't really properly named. Can you remove it?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 278 at r2 (raw file):

      !!this._requireSelectingFormattingOptions &&
      this.featureFlags.usfmFormat.enabled &&
      Date.now() < this.showSelectFormatNoticeExpireDate.getTime()

The notice should have an expiration, but the requirement to select options should not.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 285 at r2 (raw file):

  @Input() set isLatestBuild(value: boolean) {
    this._isLatestBuild = value;
    if (this._requireSelectingFormattingOptions != null) return;

This early return is a really an obfuscated way of adding a second condition to the if statement that follows. I also don't understand why it's needed. Imagine the build is the latest build, but after some time passes it ceases to be the latest. Shouldn't it then no longer require selecting formatting options?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 286 at r2 (raw file):

    this._isLatestBuild = value;
    if (this._requireSelectingFormattingOptions != null) return;
    if (this.activatedProjectService.projectDoc != null) {

Don't you mean projectDoc?.data != null?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.scss line 36 at r2 (raw file):

  button {
    display: flex;

What is the purpose of display: flex?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.scss line 41 at r2 (raw file):

  .new-label {
    color: var(--draft-history-entry-new-label);

How about throwing a font-weight: 500 on this as well?

[RaymondLuong3]

Reviewable status: 1 of 6 files reviewed, 6 unresolved discussions (waiting on @Nateowami)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html line 31 at r2 (raw file):

Previously, Nateowami wrote…

These should be spans, not divs. Neither has any semantic meaning, but spans are inline elements, and divs are block elements.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.scss line 36 at r2 (raw file):

Previously, Nateowami wrote…

What is the purpose of display: flex?

Good catch. Removed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.scss line 41 at r2 (raw file):

Previously, Nateowami wrote…

How about throwing a font-weight: 500 on this as well?

done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 273 at r2 (raw file):

Previously, Nateowami wrote…

This private property is causing more complexity than it is solving and isn't really properly named. Can you remove it?

Yes, I can do that. I've made it so that a property called formattingOptionsSelected returns true whenever the project does not have usfmConfig set on it.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 278 at r2 (raw file):

Previously, Nateowami wrote…

The notice should have an expiration, but the requirement to select options should not.

Good thinking. I've updated it so the notice is hidden after the timeframe.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 285 at r2 (raw file):

Previously, Nateowami wrote…

This early return is a really an obfuscated way of adding a second condition to the if statement that follows. I also don't understand why it's needed. Imagine the build is the latest build, but after some time passes it ceases to be the latest. Shouldn't it then no longer require selecting formatting options?

I've updated the code so that isLatestBuild setter does not have the logic anymore.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 286 at r2 (raw file):

Previously, Nateowami wrote…

Don't you mean projectDoc?.data != null?

I've updated the code to not require this check.

[Nateowami]

Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 278 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Good thinking. I've updated it so the notice is hidden after the timeframe.

It would be better to make this a property, not a getter. Properties don't have to be recalculated on every digest cycle, and it would probably feel weird to the user to see a notice suddenly disappear.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 281 at r3 (raw file):

  }

  private _isLatestBuild: boolean = false;

Why not just make this a property?

[RaymondLuong3]

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 278 at r2 (raw file):

Previously, Nateowami wrote…

It would be better to make this a property, not a getter. Properties don't have to be recalculated on every digest cycle, and it would probably feel weird to the user to see a notice suddenly disappear.

Aha, yes that is a good consideration. Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.ts line 281 at r3 (raw file):

Previously, Nateowami wrote…

Why not just make this a property?

Done.

[Nateowami]

@Nateowami reviewed 1 of 6 files at r2, 4 of 5 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html line 38 at r4 (raw file):

                <span class="new-label">{{ t("new") }}</span
                >&nbsp;
                <span>{{ t("change_line_breaks_and_quotation_marks") }}</span>

If you remove the <span> you can also remove the   and still have the space appear.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/_draft-history-entry-theme.scss line 10 at r4 (raw file):

  --draft-history-entry-grey-color: #{if($is-dark, hsl(0 0% 60%), hsl(0 0% 45%))};
  --draft-history-entry-subtitle-color: #{if($is-dark, lightGrey, #666)};
  --draft-history-entry-new-label: #{mat.get-theme-color($theme, primary, if($is-dark, 70, 40))};

The variable should have "color" in the name, like the others.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 277 at r4 (raw file):

  },
  "draft_history_entry": {
    "change_line_breaks_and_quotation_marks": "You can change how line breaks and quotation marks are handled so the draft is easier to edit.",

Based on latest discussions, the wording should be Options for line-breaks and quotation marks to make editing easier (no period)

[RaymondLuong3]

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/_draft-history-entry-theme.scss line 10 at r4 (raw file):

Previously, Nateowami wrote…

The variable should have "color" in the name, like the others.

Thanks. Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry.component.html line 38 at r4 (raw file):

Previously, Nateowami wrote…

If you remove the <span> you can also remove the   and still have the space appear.

Thanks for the suggestion. I kept it as-in for simplicity.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 277 at r4 (raw file):

Previously, Nateowami wrote…

Based on latest discussions, the wording should be Options for line-breaks and quotation marks to make editing easier (no period)

Done.