Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SF-3170 Show a summary message when no training books are selected #2971

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

josephmyers
Copy link
Collaborator

@josephmyers josephmyers commented Jan 22, 2025

image

I opted for a minimalistic solution here, because this seems to be a rare use case, based on a conversation with Nathaniel. We need to support it, but most target languages won't be in the NLLB. We can certainly do more with the UI -- on the Summary and Training pages -- but apparently it's not necessary to guide them down this path too forcefully.


This change is Reviewable

@josephmyers josephmyers marked this pull request as ready for review January 22, 2025 04:19
@josephmyers josephmyers added the will require testing PR should not be merged until testers confirm testing is complete label Jan 22, 2025
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.05%. Comparing base (4f60099) to head (93f0c3f).
Report is 78 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2971   +/-   ##
=======================================
  Coverage   82.05%   82.05%           
=======================================
  Files         544      544           
  Lines       31623    31623           
  Branches     5122     5139   +17     
=======================================
  Hits        25949    25949           
+ Misses       4917     4905   -12     
- Partials      757      769   +12     

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

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all 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 215 at r1 (raw file):

        <mat-card class="confirm-section mat-elevation-z2">
          @if (selectedTrainingBooksCollapsed().length === 0) {
            <span>No training books selected</span>

I18n

Copy link
Collaborator Author

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, Nateowami wrote…

I18n

Oops, thanks

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

@josephmyers josephmyers added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Jan 23, 2025
@Nateowami Nateowami added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed ready to test labels Jan 23, 2025
@Nateowami Nateowami merged commit 855c1cb into master Jan 23, 2025
17 checks passed
@Nateowami Nateowami deleted the fix/SF-3170 branch January 23, 2025 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testing complete Testing of PR is complete and should no longer hold up merging of the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants