SF-3633 Add draft import wizard #3638
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3638 +/- ##
==========================================
- Coverage 83.53% 83.23% -0.31%
==========================================
Files 610 611 +1
Lines 37503 37873 +370
Branches 6169 6202 +33
==========================================
+ Hits 31328 31522 +194
- Misses 5223 5400 +177
+ Partials 952 951 -1 ☔ View full report in Codecov by Sentry. |
pmachapman
changed the title
pmachapman
added
the
will require testing
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 10 files and made 19 comments.
Reviewable status: 10 of 21 files reviewed, 13 unresolved discussions (waiting on @pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 299 at r1 (raw file):
&& (!showOverwriteConfirmation || overwriteForm.valid)
I wouldn't have thought it would be important to consider overwrite input at this point. I seem to recall that perhaps before there was a step-selection control that was shown, and perhaps some of these sorts of guards are present so that use of the step selection control would not let a user jump into a step unless some prerequisites were met.
I suppose it isn't bad to have some of these extra guards. But they were confusing :). If we do decide to not have a step selection control, perhaps pruning some unnecessary guards may make it easier to understand and maintain the different steps in the future.
Okay, after finishing reading through this file, I perceive that there are lots of what seem to be unnecessary or nested+redundant guards on what is enabled or disabled. And I'm not aware that that is hindering functionality, but it will be a cleaner template to maintain if we don't have those. What do you think? Maybe I'm not understanding how this is working.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 192 at r1 (raw file):
<!-- Step 4: Book Selection (conditional - only if multiple books) --> @if (showBookSelection) { <mat-step [completed]="selectedBooks.length > 0" [editable]="!isConnecting && !isImporting">
I find it interesting that the steps often have this [editable]="!isConnecting && !isImporting". And sometimes, it doesn't seem like it should even be possible for the system to be connecting or importing, like here when the user is selecting books.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 264 at r1 (raw file):
@if (isConnecting || isImporting) { <app-notice type="info">
Would it really be that in the overwrite confirmation step, the project might be connecting or importing?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss line 36 at r1 (raw file):
.progress-text { font-size: 0.875rem; color: var(--mdc-theme-text-secondary-on-background, rgba(0, 0, 0, 0.6));
I've been learning more about this.
--mdc-theme-text-secondary-on-background is not defined, and so we fall back to rgba(0, 0, 0, 0.6), which is a medium transparent black.
Looking at https://material.angular.dev/guide/theming-your-components#colors , we can see that a variable to use here would be --mat-sys-on-surface, which is mostly black. So this line can preferably become:
color: var(--mat-sys-on-surface);src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss line 44 at r1 (raw file):
column-gap: 0.35rem; margin-top: 0.35rem; color: var(--mdc-theme-error, #b00020);
--mdc-theme-error is not defined, and we are falling back to the hard-coded color value. We could consider using --sf-error-foreground, but I would suggest --mat-sys-error ( https://material.angular.dev/guide/theming-your-components#text ).
So this line can preferably become:
color: var(--mat-sys-error);src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss line 82 at r1 (raw file):
mat-dialog-content { /* Use the same text color as the main application body, not the lighter dialog text color */ color: var(--mat-sidenav-content-text-color, var(--mat-sys-on-background)) !important;
Were you finding that the default text color in the dialog was difficult to see, or did not seem appropriate for the task?
It looks like the default text color may be var(--mat-dialog-supporting-text-color, var(--mat-sys-on-surface-variant, rgba(0, 0, 0, 0.6))) (semi-purplish grey, falling back to semi-purplish grey, falling back to medium transparent black). And here we set it to var(--mat-sidenav-content-text-color, var(--mat-sys-on-background)) (close to black, falling back to close to black).
It looks like the dialog background color is set in material-styles.scss:
@include mat.dialog-overrides(
(
container-max-width: 80vw,
container-color: mat.get-theme-color($theme, neutral, if($is-dark, 20, 100))
)
);where it is set to the value in _default.scss neutral 100, which is #ffffff.
I think --mat-sys-on-surface would be a good choice for the text color specification here. That has an identical color value to what you are specifying, but I think is a bit more apt of a source from which to get the color. (Tho --mat-sys-on-background isn't bad. )
So it's probably not super important, but what do you think about here instead using:
color: var(--mat-sys-on-surface);I would also suggest that instead of using a !important here, that we use a dialog style override ( https://material.angular.dev/components/dialog/styling ). We can override the supporting-text-color with the desired color. Consider for example, the following change that makes use of that:
.../draft-import-wizard/_draft-import-wizard-theme.scss | 13 +++++++++++++
.../draft-import-wizard/draft-import-wizard.component.scss | 5 -----
src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss | 2 ++
3 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/_draft-import-wizard-theme.scss b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/_draft-import-wizard-theme.scss
new file mode 100644
index 000000000..7c6d5a5bb
--- /dev/null
+++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/_draft-import-wizard-theme.scss
@@ -0,0 +1,13 @@
+@use '@angular/material' as mat;
+
+@mixin theme($theme) {
+ $is-dark: mat.get-theme-type($theme) == dark;
+
+ .mat-mdc-dialog-container {
+ @include mat.dialog-overrides(
+ (
+ supporting-text-color: var(--mat-sys-on-surface)
+ )
+ );
+ }
+}
diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss
index b58a465fe..ae6a3cfe0 100644
--- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss
+++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss
@@ -77,11 +77,6 @@ app-sync-progress {
}
::ng-deep {
- mat-dialog-content {
- /* Use the same text color as the main application body, not the lighter dialog text color */
- color: var(--mat-sidenav-content-text-color, var(--mat-sys-on-background)) !important;
- }
-
/* Hide the wizard headings at the top of the dialog */
mat-stepper .mat-horizontal-stepper-header-container {
display: none;
diff --git a/src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss b/src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss
index 20d5622f4..05265ea4e 100644
--- a/src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss
+++ b/src/SIL.XForge.Scripture/ClientApp/src/material-styles.scss
@@ -17,6 +17,7 @@
@use 'src/app/translate/draft-generation/draft-history-list/draft-history-entry/draft-history-entry-theme' as
sf-draft-history-entry;
@use 'src/app/translate/draft-generation/draft-usfm-format/draft-usfm-format-theme' as sf-draft-usfm-format;
+@use 'src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard-theme' as sf-draft-import-wizard;
@use 'src/app/translate/editor/editor-theme' as sf-editor;
@use 'src/app/translate/editor/note-dialog/note-dialog-theme' as sf-note-dialog;
@use 'src/app/translate/editor/editor-draft/editor-draft-theme' as sf-editor-draft;
@@ -45,6 +46,7 @@
@include sf-confirm-sources.theme($theme);
@include sf-draft-generation-steps.theme($theme);
@include sf-draft-history-entry.theme($theme);
+ @include sf-draft-import-wizard.theme($theme);
@include sf-draft-sources.theme($theme);
@include sf-draft-usfm-format.theme($theme);
@include sf-editor.theme($theme);src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss line 87 at r1 (raw file):
/* Hide the wizard headings at the top of the dialog */ mat-stepper .mat-horizontal-stepper-header-container { display: none;
Interesting that Angular Material doesn't appear to provide a way to omit the stepper!
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 170 at r1 (raw file):
} if (!skipStepperAdvance) {
I am not excited about how this mixes (1) controlling user flow thru the dialog, with (2) what seems like it could otherwise be a pretty contained library method, "connect To Project" and not know anything about the user interface.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 175 at r1 (raw file):
this.connectionError = undefined; this.isConnecting = true;
With the many checks of isConnecting, I would think we would want to make sure this is above this.stepper?.next();. It seems like the template in step three will benefit from setting isConnecting first before entering step 3.
But maybe it doesn't matter?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 191 at r1 (raw file):
// Reload project data after connection const projectDoc = await this.projectService.get(this.targetProjectId); this.targetProjectDoc$.next(projectDoc);
I'm confused about why we emit targetProjectDoc$ here and then again in analyzeTargetProject.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 192 at r1 (raw file):
const projectDoc = await this.projectService.get(this.targetProjectId); this.targetProjectDoc$.next(projectDoc); if (projectDoc.data != null) {
And if we do emit targetProjectDoc$ in connectToProject() I am slightly uncomfortable not also emitting targetProject$ (since they would be out of sync.. at least at first).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 193 at r1 (raw file):
this.targetProjectDoc$.next(projectDoc); if (projectDoc.data != null) { await this.analyzeTargetProject(projectDoc.data, paratextProject.isConnected);
I think the method name analyzeTargetProject could be improved. "analyzeTargetProject" is not very illuminating regarding what will actually happen if we do or don't analyze it, or if we might benefit from doing it more than once.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 197 at r1 (raw file):
try { await this.analyzeBooksForOverwrite();
That is handy that this method name tells us why we might call it. But I think a better method name will be to tell us what the method is doing, and the caller can decide why to call it. Perhaps something like determineBooksAndChaptersWithText()?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 199 at r1 (raw file):
await this.analyzeBooksForOverwrite(); } catch (analysisError) { console.error('Failed to analyze books for overwrite', analysisError);
What are we thinking might go wrong here?
Calling this.projectService.getText? I'm wondering if it should be an error that we surface to the user rather than just print it to the console, if in fact something may be going wrong here.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 215 at r1 (raw file):
this.stepper?.next(); if (this.targetProjectId != null) {
It looks like from projectService.onlineCreate, that it will return a string, not a string|undefined. So we shouldn't need to check for null here.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 217 at r1 (raw file):
if (this.targetProjectId != null) { const projectDoc = await this.projectService.get(this.targetProjectId); this.targetProjectDoc$.next(projectDoc);
Hmm, but not also emitting targetProject$. I don't like that we have both and they aren't necessarily kept in sync :)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 567 at r1 (raw file):
private async validateProject(): Promise<void> { await new Promise<void>(resolve => {
Okay, and this way of calling customValidate is so that it's done in a following macrotask? And so that something in the form or current macrotask can finish first?
src/SIL.XForge.Scripture/ClientApp/src/app/core/project-notification.service.ts line 33 at r1 (raw file):
setNotifyDraftApplyProgressHandler(handler: any): void { this.connection.on('notifyDraftApplyProgress', handler);
Might we be accruing connection handlers that might be something we should be letting go of with off()?
src/SIL.XForge.Scripture/Services/MachineApiService.cs line 379 at r1 (raw file):
return result; }
It looks like this method could benefit from being split up into smaller methods some time. I haven't thought about the method variables to consider if that would be a pain and more messy.
This PR adds a new draft import wizard that uses the backend to adds the books and chapters to the project specified.
This change is