Update line break draft format wording #3273

RaymondLuong3 · 2025-06-17T17:27:08Z

This PR updates the wording for the line break options in the draft format component. Additionally, the user now navigates back to the generate draft page after saving the changes which is more intuitive. A progress circle has been introduced to notify the user their changes are currently saving.
I marked this as testing not required since the changes can be tested by a developer.

This change is

codecov · 2025-06-17T17:36:49Z

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.52%. Comparing base (9f6b920) to head (8235f34).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...n/draft-usfm-format/draft-usfm-format.component.ts	80.00%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3273   +/-   ##
=======================================
  Coverage   82.52%   82.52%           
=======================================
  Files         605      605           
  Lines       35196    35205    +9     
  Branches     5715     5697   -18     
=======================================
+ Hits        29045    29053    +8     
- Misses       5299     5312   +13     
+ Partials      852      840   -12

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

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Nateowami

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @RaymondLuong3)

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 182 at r1 (raw file):

      await this.projectService.onlineSetUsfmConfig(this.projectId, this.currentFormat);
      this.lastSavedState = this.currentFormat;
      // not awaited so that the user is directed to the draft generation page

I don't understand this comment. How does awaiting prevent a redirect after saving?

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 185 at r1 (raw file):

      this.servalAdministration.onlineRetrievePreTranslationStatus(this.projectId).then(() => {
        this.router.navigate(['projects', this.projectId, 'draft-generation']);
        this.noticeService.show(translate('draft_usfm_format.changes_have_been_saved'));

Given the redirect only happens if it's successful, I think the notice is unneeded and doesn't match what we do elsewhere.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html line 65 at r1 (raw file):

            {{ t("save_changes") }}
          </button>
          <div class="progress" [ngClass]="{ saving: saving }">

What about [class.saving]="saving"?

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html line 66 at r1 (raw file):

          </button>
          <div class="progress" [ngClass]="{ saving: saving }">
            <mat-spinner diameter="24"></mat-spinner>

I really dislike how this spinner changes the layout, but I don't have anything great to suggest. My best idea is to replace the check mark with a spinner, but making that work is somewhat challenging.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.scss line 92 at r1 (raw file):

  justify-content: flex-end;

  .progress {

How about:

.progress:not(.saving) {
  visibility: hidden;
}

Only one rule needed, and the intent is clearer.

RaymondLuong3

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Nateowami)

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html line 65 at r1 (raw file):

Previously, Nateowami wrote…

What about [class.saving]="saving"?

Good idea. Done.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html line 66 at r1 (raw file):

Previously, Nateowami wrote…

I really dislike how this spinner changes the layout, but I don't have anything great to suggest. My best idea is to replace the check mark with a spinner, but making that work is somewhat challenging.

I agree. I also did not like how adding the spinner means we have to adjust the layout for the action buttons. But I think the spinner is worth keeping to communicate that something happened when the user clicked save.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.scss line 92 at r1 (raw file):

Previously, Nateowami wrote…

How about:
.progress:not(.saving) {
  visibility: hidden;
}
Only one rule needed, and the intent is clearer.

Done.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 182 at r1 (raw file):

Previously, Nateowami wrote…

I don't understand this comment. How does awaiting prevent a redirect after saving?

Thanks for catching this. The comment was obsolete. I've updated it.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 185 at r1 (raw file):

Previously, Nateowami wrote…

Given the redirect only happens if it's successful, I think the notice is unneeded and doesn't match what we do elsewhere.

That would work. If the user clicks save, that is explicit enough that we will save their changes.

Nateowami

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

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html line 66 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I agree. I also did not like how adding the spinner means we have to adjust the layout for the action buttons. But I think the spinner is worth keeping to communicate that something happened when the user clicked save.

Can you:

Space cancel and save as far apart as possible (justify-content: space-between;)
Put the spinner and save button in a wrapper
Put the spinner to the left of the save button

I think that would look a lot better and not shift the layout.

RaymondLuong3

Reviewable status: 4 of 7 files reviewed, 1 unresolved discussion (waiting on @Nateowami)

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.html line 66 at r1 (raw file):

Previously, Nateowami wrote…

Can you:

Space cancel and save as far apart as possible (justify-content: space-between;)

Put the spinner and save button in a wrapper

Put the spinner to the left of the save button

I think that would look a lot better and not shift the layout.

How does this look?

Nateowami

Reviewed 1 of 6 files at r2, 2 of 2 files at r3, 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-usfm-format/draft-usfm-format.component.html line 66 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

How does this look?

Looks a lot better! Thanks!

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 185 at r3 (raw file):

      this.servalAdministration
        .onlineRetrievePreTranslationStatus(this.projectId)
        .then(() => this.router.navigate(['projects', this.projectId, 'draft-generation']));

Is it intentional that you're not showing the error message when retrieving pretranslations fails? The normal way to write this would be:

      await this.servalAdministration.onlineRetrievePreTranslationStatus(this.projectId);
      this.router.navigate(['projects', this.projectId, 'draft-generation']);

By not using await, you're not catching the error in the catch clause.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 186 at r3 (raw file):

        .onlineRetrievePreTranslationStatus(this.projectId)
        .then(() => this.router.navigate(['projects', this.projectId, 'draft-generation']));
    } catch {

Errors that are caught should also be reported, ideally to Bugsnag, but at a minimum logged to the console.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 187 at r3 (raw file):

        .then(() => this.router.navigate(['projects', this.projectId, 'draft-generation']));
    } catch {
      this.noticeService.showError(translate('draft_usfm-format.failed_to_save'));

This translation key does not exist. If you used this.i18n.translateStatic() instead of the bare translate(), it would have been obvious, since it provides type safety.

RaymondLuong3

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Nateowami)

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 185 at r3 (raw file):

Previously, Nateowami wrote…

Is it intentional that you're not showing the error message when retrieving pretranslations fails? The normal way to write this would be:
      await this.servalAdministration.onlineRetrievePreTranslationStatus(this.projectId);
      this.router.navigate(['projects', this.projectId, 'draft-generation']);
By not using await, you're not catching the error in the catch clause.

No, that was not intentional. Thanks for catching that.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 186 at r3 (raw file):

Previously, Nateowami wrote…

Errors that are caught should also be reported, ideally to Bugsnag, but at a minimum logged to the console.

I'll log it to the console. The backend already has error reporting built in.

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 187 at r3 (raw file):

Previously, Nateowami wrote…

This translation key does not exist. If you used this.i18n.translateStatic() instead of the bare translate(), it would have been obvious, since it provides type safety.

Done.

Nateowami

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 186 at r3 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I'll log it to the console. The backend already has error reporting built in.

Please actually catch and log the error, rather than just logging a string. The error message you've added isn't even necessarily correct; there are multiple things that can throw in this context.

RaymondLuong3

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

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format.component.ts line 186 at r3 (raw file):

Previously, Nateowami wrote…

Please actually catch and log the error, rather than just logging a string. The error message you've added isn't even necessarily correct; there are multiple things that can throw in this context.

Good suggestion. Done.

Nateowami

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

RaymondLuong3 added the testing not required label Jun 17, 2025

Nateowami requested changes Jun 17, 2025

View reviewed changes

RaymondLuong3 commented Jul 3, 2025

View reviewed changes

RaymondLuong3 force-pushed the feature/draft-format-wording branch from d4e4cab to 634d929 Compare July 3, 2025 22:29

Nateowami reviewed Jul 7, 2025

View reviewed changes

RaymondLuong3 force-pushed the feature/draft-format-wording branch from 634d929 to cad6289 Compare July 8, 2025 19:28

RaymondLuong3 commented Jul 8, 2025

View reviewed changes

Nateowami requested changes Jul 10, 2025

View reviewed changes

Nateowami and others added 5 commits July 10, 2025 10:49

Improve draft format wording based on meeting 2025-06-16

b9fe961

Improve wording for draft format

dfbdc8e

Show save progress circle

325b31f

Code review fixes

49fb99e

Improve action buttons layout

f780ddd

RaymondLuong3 commented Jul 10, 2025

View reviewed changes

Catch errors if webhook status fails

8e25b75

RaymondLuong3 force-pushed the feature/draft-format-wording branch from cad6289 to 8e25b75 Compare July 10, 2025 17:02

Nateowami requested changes Jul 10, 2025

View reviewed changes

Show error on console

8235f34

RaymondLuong3 force-pushed the feature/draft-format-wording branch from fbb7fd9 to 8235f34 Compare July 10, 2025 23:36

RaymondLuong3 commented Jul 10, 2025

View reviewed changes

Nateowami approved these changes Jul 11, 2025

View reviewed changes

Nateowami merged commit 1a9bad1 into master Jul 11, 2025
18 checks passed

Nateowami deleted the feature/draft-format-wording branch July 11, 2025 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update line break draft format wording #3273

Update line break draft format wording #3273

RaymondLuong3 commented Jun 17, 2025 •

edited

Loading

Uh oh!

codecov bot commented Jun 17, 2025 •

edited

Loading

Uh oh!

Nateowami left a comment

Uh oh!

RaymondLuong3 left a comment

Uh oh!

Nateowami left a comment

Uh oh!

RaymondLuong3 left a comment

Uh oh!

Nateowami left a comment

Uh oh!

RaymondLuong3 left a comment

Uh oh!

Nateowami left a comment

Uh oh!

RaymondLuong3 left a comment

Uh oh!

Nateowami left a comment

Uh oh!

Uh oh!

Update line break draft format wording #3273

Update line break draft format wording #3273

Conversation

RaymondLuong3 commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Nateowami left a comment

Choose a reason for hiding this comment

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

Nateowami left a comment

Choose a reason for hiding this comment

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

Nateowami left a comment

Choose a reason for hiding this comment

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

Nateowami left a comment

Choose a reason for hiding this comment

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

Nateowami left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RaymondLuong3 commented Jun 17, 2025 •

edited

Loading

codecov bot commented Jun 17, 2025 •

edited

Loading