-
-
Notifications
You must be signed in to change notification settings - Fork 5
SF-3649 Add UI to filter projects with custom serval config #3577
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3577 +/- ##
==========================================
- Coverage 82.83% 82.82% -0.01%
==========================================
Files 610 610
Lines 37184 37192 +8
Branches 6096 6092 -4
==========================================
+ Hits 30800 30804 +4
- Misses 5460 5461 +1
- Partials 924 927 +3 ☔ View full report in Codecov by Sentry. |
f88f7d1 to
a82a151
Compare
a82a151 to
8b454cd
Compare
pmachapman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmachapman reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 9 at r1 (raw file):
<mat-checkbox [(ngModel)]="showProjectsWithCustomServalConfig" (change)="updateServalConfigFilter()" >Projects with custom serval configs only</mat-checkbox >
NIT: Could you make this look tidier by adding spaces before and after the caption, i.e.
<mat-checkbox [(ngModel)]="showProjectsWithCustomServalConfig" (change)="updateServalConfigFilter()">
Projects with custom serval configs only
</mat-checkbox>Also, "serval configs" should be "Serval configuration" (this is the wording used on the Serval admin page)
Finally, would you be able to make this display beside the filter projects box rather than below?
Code quote:
<mat-checkbox [(ngModel)]="showProjectsWithCustomServalConfig" (change)="updateServalConfigFilter()"
>Projects with custom serval configs only</mat-checkbox
>src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html line 314 at r1 (raw file):
<u (click)="showCustomConfig = !showCustomConfig">{{ showCustomConfig ? t("hide_custom_serval_config") : t("show_custom_config") }}</u>
Can you please make this more like the "Show model training configuration" toggle on the Generate draft page? i.e.
<div>
<a href="javascript:;" (click)="showCustomConfig = !showCustomConfig" class="serval-config-toggle">
<mat-icon>expand_{{ showCustomConfig ? "less" : "more" }}</mat-icon>
{{ t(showCustomConfig ? "hide_custom_serval_config" : "show_custom_config") }}
</a>
</div>CSS:
.serval-config-toggle {
display: flex;
align-items: center;
}Code quote:
<div class="serval-config-toggle">
<u (click)="showCustomConfig = !showCustomConfig">{{
showCustomConfig ? t("hide_custom_serval_config") : t("show_custom_config")
}}</u>src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html line 317 at r1 (raw file):
</div> @if (showCustomConfig) { <pre class="custom-config">{{ servalConfig | json }}</pre>
There is a app-json-viewer component which will display the JSON nicely. Are you able to use that here?
Also, the JSON string displayed with double quotes escaped (is it possible to have it display without escaped characters?):
Code quote:
<pre class="custom-config">{{ servalConfig | json }}</pre>src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 249 at r1 (raw file):
"fast_training": "Enable Fast Training", "generate_draft": "Generate draft", "hide_custom_serval_config": "Hide custom serval configuration",
The user won't know what Serval is. Can you maybe call it "custom draft configuration"?
Code quote:
Hide custom serval configurationsrc/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 260 at r1 (raw file):
"remote_changes": "The source configuration for draft generation has been modified. Please start a new draft and review the latest source configuration.", "remote_changes_start_over": "Start over", "show_custom_config": "Show custom serval configuration",
Also here: the user won't know what Serval is.
Code quote:
Show custom serval configuration
RaymondLuong3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @pmachapman)
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 9 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
NIT: Could you make this look tidier by adding spaces before and after the caption, i.e.
<mat-checkbox [(ngModel)]="showProjectsWithCustomServalConfig" (change)="updateServalConfigFilter()"> Projects with custom serval configs only </mat-checkbox>Also, "serval configs" should be "Serval configuration" (this is the wording used on the Serval admin page)
Finally, would you be able to make this display beside the filter projects box rather than below?
Thanks. I also updated the checkbox to come on the same line. I moved it to the end, but let me know if you would rather it come directly next to the filter box.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html line 314 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Can you please make this more like the "Show model training configuration" toggle on the Generate draft page? i.e.
<div> <a href="javascript:;" (click)="showCustomConfig = !showCustomConfig" class="serval-config-toggle"> <mat-icon>expand_{{ showCustomConfig ? "less" : "more" }}</mat-icon> {{ t(showCustomConfig ? "hide_custom_serval_config" : "show_custom_config") }} </a> </div>CSS:
.serval-config-toggle { display: flex; align-items: center; }
Thanks for the suggestion. My memory is that we decided to not show the custom config and just add a notice. I have made that change.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html line 317 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
There is a
app-json-viewercomponent which will display the JSON nicely. Are you able to use that here?Also, the JSON string displayed with double quotes escaped (is it possible to have it display without escaped characters?):
This won't be needed anymore. See above.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 249 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
The user won't know what Serval is. Can you maybe call it "custom draft configuration"?
Removed.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 260 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Also here: the user won't know what Serval is.
Removed.
8b454cd to
ebfc3c9
Compare
pmachapman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmachapman reviewed 8 of 8 files at r2, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/serval-administration/serval-projects.component.html line 9 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Thanks. I also updated the checkbox to come on the same line. I moved it to the end, but let me know if you would rather it come directly next to the filter box.
That's great - thanks!
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation-steps/draft-generation-steps.component.html line 314 at r1 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
Thanks for the suggestion. My memory is that we decided to not show the custom config and just add a notice. I have made that change.
Sounds good - I had forgotten that.



This PR adds a way to filter projects on the serval admin page and show projects with custom serval configuration set. This also adds UI to show the user that custom serval configuration is set on the summary page of the draft steps.
This change is