Conversation
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.
pmachapman
left a comment
pmachapman
left a comment
There was a problem hiding this comment.
@marksvc Thank you for your hard work reviewing this! Your comments have been really helpful.
@pmachapman made 14 comments.
Reviewable status: 10 of 21 files reviewed, 13 unresolved discussions (waiting on @marksvc).
src/SIL.XForge.Scripture/ClientApp/src/app/core/project-notification.service.ts line 33 at r1 (raw file):
Previously, marksvc wrote…
Might we be accruing connection handlers that might be something we should be letting go of with off()?
Yes, we are. Good point. I have fixed this for this handler and the other SignalR notification handlers.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 264 at r1 (raw file):
Previously, marksvc wrote…
Would it really be that in the overwrite confirmation step, the project might be connecting or importing?
I think this was only an issue when the stepper steps could be clicked between. Removed.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 299 at r1 (raw file):
Previously, marksvc wrote…
&& (!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.
[editable] was necessary when the stepper steps could be clicked between, but I don't think it does anything useful now. Removed.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss line 36 at r1 (raw file):
Previously, marksvc wrote…
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);
I've made it --mat-sys-on-surface-variant, which I think is the most appropriate.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss line 44 at r1 (raw file):
Previously, marksvc wrote…
--mdc-theme-erroris 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);
Done. Thank you!
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.scss line 82 at r1 (raw file):
Were you finding that the default text color in the dialog was difficult to see, or did not seem appropriate for the task?
Yes. Thank you - that is a lot better. Angular theming is something I need to learn more about.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 175 at r1 (raw file):
Previously, marksvc wrote…
With the many checks of
isConnecting, I would think we would want to make sure this is abovethis.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?
I don't think it matters practically, but as step 2 displays the next button as disabled if isConnecting is true, I think it makes logical sense to leave as is.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 191 at r1 (raw file):
Previously, marksvc wrote…
I'm confused about why we emit targetProjectDoc$ here and then again in analyzeTargetProject.
It is emitted here to update the UI. It is emitted in analyzeTargetProject as one method which calls this gets the profile which cannot be emitted via targetProjectDoc$, then passes that to analyzeTargetProject which sees if the user can load the project properly, does so, then emits targetProjectDoc$.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 193 at r1 (raw file):
Previously, marksvc wrote…
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.
Yes, I thought so too, but struggled to rename. How about loadTargetProjectAndValidate?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 197 at r1 (raw file):
Previously, marksvc wrote…
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()?
Sure. Sounds good.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 199 at r1 (raw file):
Previously, marksvc wrote…
What are we thinking might go wrong here?
Callingthis.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.
I think the try catches should be removed. Something to go wrong would be connection or permission, or deletion issues. I think this was added for debugging during development?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 215 at r1 (raw file):
Previously, marksvc wrote…
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.
Sounds good. Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 567 at r1 (raw file):
Previously, marksvc wrote…
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?
This is the pattern used in DraftApplyDialogComponent.validateProject() - I've copied the comment from there.
pmachapman
force-pushed
the
feature/SF-3633
branch
2 times, most recently
from
cf51728 to
3516e34
Compare
|
Hey, when I make "Informing" comments like this, do you see them, or I wonder if the tool starts them out in a closed state and they are not seen :). |
|
When I make "Informing" comments like the one on the line above, do you see them, or I wonder if the tool starts them out in a closed state and they are not seen :). |
|
Previously, marksvc wrote…
(That is, in Reviewable, not in the GitHub comment threads.) |
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@marksvc partially reviewed 10 files, made 13 comments, and resolved 10 discussions.
Reviewable status: 12 of 25 files reviewed, 10 unresolved discussions (waiting on @pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 175 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
I don't think it matters practically, but as step 2 displays the next button as disabled if
isConnectingistrue, I think it makes logical sense to leave as is.
Good point. Thank you.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 193 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Yes, I thought so too, but struggled to rename. How about
loadTargetProjectAndValidate?
Good. Thank you.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 138 at r1 (raw file):
targetProjectDoc$ = new BehaviorSubject<SFProjectDoc | undefined>(undefined); canEditProject = true; booksMissingWithoutPermission = false;
I inferred the meaning of this from the localization file. I think we will be helped by adding a comment. Perhaps:
// Text is to be imported to books in the target project which do not exist, and the user does not have permission to create those books.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 233 at r1 (raw file):
} updateConnectStatus(inProgress: boolean): void {
I don't like how the previous step initiates with onlineCreate, and in the current step, the sync progress component sends $event to updateConnectStatus(), which then calls loadTargetProojectAndValidate(). I think I don't like it both because of how UI component activity is mixed with behind the scenes data activity. And because we finish connecting up to a project in response to <app-sync-progress (inProgress), a UI component's event, rather than from watching something behind the UI.
I see that SyncProgressComponent is written to be used for this sort of thing, so maybe I should just not frown and continue :)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 235 at r2 (raw file):
} void this.loadTargetProjectAndValidate(projectDoc.data, false)
Shouldn't we pass true for arg isConnected, not false?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 237 at r2 (raw file):
void this.loadTargetProjectAndValidate(projectDoc.data, false) .then(async () => { return await this.determineBooksAndChaptersWithText();
Is there a reason to prefer this instead writing something like the following?
try {
await this.loadTargetProjectAndValidate(projectDoc.data, false);
await this.determineBooksAndChaptersWithText();
} finally {
this.isConnecting = false;
}src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 416 at r2 (raw file):
private async loadSourceProjectContext(): Promise<void> { this.sourceProjectId = this.activatedProjectService.projectId; if (this.sourceProjectId == null) {
This is an interesting condition to consider. I wonder if in the situation where the user does not have an active project selected in SF, then we should just return. It seems strange to go on to consider availableBooksforImport.length. But maybe I am not correctly thinking about what the purpose of this method is.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 490 at r2 (raw file):
// Connection status comes from ParatextProject this.needsConnection = !isConnected && this.canEditProject;
That's interesting that if the project is not connected, and the user can't edit the project, then no, we don't needsConnection :)
I didn't try this scenario to see what happens.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 492 at r2 (raw file):
this.needsConnection = !isConnected && this.canEditProject; if (this.canEditProject && this.targetProjectId != null) {
So, we receive an argument (project) for the target project. And here we have a condition on whether we do or don't have its SF project id recorded. :-$. If I were to trace it all out I would probably see that it fits together correctly.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 498 at r2 (raw file):
this.targetProjectDoc$.next(projectDoc); } else { this.targetProject$.next(undefined);
(And the above gets to an interesting potential logic branch where the user has edit access to a project at SF, but we didn't have the SF project id recorded, and so we emit undefined for the target project.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 513 at r2 (raw file):
if (project == null) { const profileDoc = await this.projectService.getProfile(this.targetProjectId); project = profileDoc.data ?? undefined;
I think we can omit ?? undefined?
Code quote:
?? undefined;src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 515 at r2 (raw file):
project = profileDoc.data ?? undefined; if (project != null) { this.targetProject$.next(project);
It seems unfortunate that ensureSelectedBooksExist is a place where we check if targetProject$ has a value, and if not we getProfile and next it.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 537 at r2 (raw file):
if (!canCreateBooks) { this.booksMissingWithoutPermission = true; const booksDescription = this.missingBookNames.length > 0 ? ` (${this.missingBookNames.join(', ')})` : '';
I can we can change this line to
... =` (${this.missingBookNames.join(', ')})`;since we know that missingBooks.length !== 0 at this point (and missingBookNames.length should === missingBooks.length).
pmachapman
left a comment
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman made 7 comments and resolved 1 discussion.
Reviewable status: 12 of 25 files reviewed, 9 unresolved discussions (waiting on @marksvc).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 138 at r1 (raw file):
Previously, marksvc wrote…
I inferred the meaning of this from the localization file. I think we will be helped by adding a comment. Perhaps:
// Text is to be imported to books in the target project which do not exist, and the user does not have permission to create those books.
No worries. Is this codedoc comment OK?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 235 at r2 (raw file):
Previously, marksvc wrote…
Shouldn't we pass
truefor argisConnected, notfalse?
I think you are right. needsConnection is only used on step 1/2 so this didn't show up as a bug, as this code is used on step 3
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 237 at r2 (raw file):
Previously, marksvc wrote…
Is there a reason to prefer this instead writing something like the following?
try { await this.loadTargetProjectAndValidate(projectDoc.data, false); await this.determineBooksAndChaptersWithText(); } finally { this.isConnecting = false; }
updateConnectStatus should not be async, so the then() is used. It also means that the async calkl doesn't block updateConnectStatus.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 416 at r2 (raw file):
Previously, marksvc wrote…
This is an interesting condition to consider. I wonder if in the situation where the user does not have an active project selected in SF, then we should just return. It seems strange to go on to consider availableBooksforImport.length. But maybe I am not correctly thinking about what the purpose of this method is.
I think this method is stuck with old logic while everything else has passed it by. I've removed it and moved the logic into ngOnInit.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 504 at r2 (raw file):
Previously, marksvc wrote…
(That is, in Reviewable, not in the GitHub comment threads.)
I don't see a comment???
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 513 at r2 (raw file):
Previously, marksvc wrote…
I think we can omit
?? undefined?
Yes. I've also made the logic a bit clearer.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 537 at r2 (raw file):
Previously, marksvc wrote…
I can we can change this line to
... =` (${this.missingBookNames.join(', ')})`;since we know that missingBooks.length !== 0 at this point (and missingBookNames.length should === missingBooks.length).
Good spotting. Thank you!
pmachapman
force-pushed
the
feature/SF-3633
branch
3 times, most recently
from
3288c01 to
084c129
Compare
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 1 file, made 12 comments, and resolved 8 discussions.
Reviewable status: 13 of 25 files reviewed, 11 unresolved discussions (waiting on @pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 138 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
No worries. Is this codedoc comment OK?
Good.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 170 at r1 (raw file):
Previously, marksvc wrote…
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.
(Just saying. Converting to 'Discussing' so it's seen.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 217 at r1 (raw file):
Previously, marksvc wrote…
Hmm, but not also emitting targetProject$. I don't like that we have both and they aren't necessarily kept in sync :)
(Just saying. Converting to 'Discussing' so it's seen.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 233 at r1 (raw file):
Previously, marksvc wrote…
I don't like how the previous step initiates with onlineCreate, and in the current step, the sync progress component sends $event to updateConnectStatus(), which then calls loadTargetProojectAndValidate(). I think I don't like it both because of how UI component activity is mixed with behind the scenes data activity. And because we finish connecting up to a project in response to
<app-sync-progress (inProgress), a UI component's event, rather than from watching something behind the UI.
I see that SyncProgressComponent is written to be used for this sort of thing, so maybe I should just not frown and continue :)
(Just saying. Converting to 'Discussing' so it's seen.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 490 at r2 (raw file):
Previously, marksvc wrote…
That's interesting that if the project is not connected, and the user can't edit the project, then no, we don't
needsConnection:)I didn't try this scenario to see what happens.
(Just saying. Converting to 'Discussing' so it's seen.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 492 at r2 (raw file):
Previously, marksvc wrote…
So, we receive an argument (
project) for the target project. And here we have a condition on whether we do or don't have its SF project id recorded. :-$. If I were to trace it all out I would probably see that it fits together correctly.
(Just saying. Converting to 'Discussing' so it's seen.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 498 at r2 (raw file):
Previously, marksvc wrote…
(And the above gets to an interesting potential logic branch where the user has edit access to a project at SF, but we didn't have the SF project id recorded, and so we emit undefined for the target project.)
(Just saying. Converting to 'Discussing' so it's seen.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 503 at r2 (raw file):
Previously, marksvc wrote…
Hey, when I make "Informing" comments like this, do you see them, or I wonder if the tool starts them out in a closed state and they are not seen :).
Well, I guess it's not very useful to make "Informing" comments, then, if they are not seen :)
(Converting to 'Discussing' so it's seen.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 504 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
I don't see a comment???
Hmm. Okay; I've adjusted my disposition for them from Informing to Discussing so they show up.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 515 at r2 (raw file):
Previously, marksvc wrote…
It seems unfortunate that
ensureSelectedBooksExistis a place where we check iftargetProject$has a value, and if not wegetProfileandnextit.
(Just saying. Converting to 'Discussing' so it's seen.)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 611 at r4 (raw file):
} // For connected projects, ensure we have a targetProjectId before proceeding
I wonder what the user sees when this happens, as here we just return from the method without seeming to tell the user anything.
📌 Just saying.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 57 at r1 (raw file):
@if (projectSelectionForm.valid && !isLoadingProject && (targetProject$ | async); as project) { <app-notice [icon]="'verified'"> {{ t("ready_to_import", { projectShortName: $any(project).shortName, projectName: $any(project).name }) }}
I did some investigation and experimenting regarding these $any (which I perceive as a type-unsafe cast).
It looks like because (targetProject$ | async); as project is in the @if (...), we won't do the condition if (targetProject$ | async) is not truthy. And so if project is undefined, then we don't go into the if block and use project.
It looks like we can simply remove the $any casts. For example, the following diff.
Though maybe the $any is doing something else that I am overlooking?
@@ -50,7 +50,7 @@
@if (projectSelectionForm.valid && !isLoadingProject && (targetProject$ | async); as project) {
<app-notice [icon]="'verified'">
- {{ t("ready_to_import", { projectShortName: $any(project).shortName, projectName: $any(project).name }) }}
+ {{ t("ready_to_import", { projectShortName: project.shortName, projectName: project.name }) }}
</app-notice>
}
@@ -130,10 +130,7 @@
@if (isConnecting) {
<h1>{{ t("connecting_to_project") }}</h1>
@if (targetProjectDoc$ | async; as projectDoc) {
- <app-sync-progress
- [projectDoc]="$any(projectDoc)"
- (inProgress)="updateConnectStatus($event)"
- ></app-sync-progress>
+ <app-sync-progress [projectDoc]="projectDoc" (inProgress)="updateConnectStatus($event)"></app-sync-progress>
} @else {
<p>{{ t("setting_up_project", { projectName: selectedParatextProject?.name }) }}</p>
<mat-progress-bar mode="indeterminate" color="accent"></mat-progress-bar>
@@ -157,7 +154,7 @@
<h1>{{ t("connected_to_project") }}</h1>
@if (targetProject$ | async; as project) {
<app-notice [icon]="'verified'">
- {{ t("ready_to_import", { projectShortName: $any(project).shortName, projectName: $any(project).name }) }}
+ {{ t("ready_to_import", { projectShortName: project.shortName, projectName: project.name }) }}
</app-notice>
}
<div class="button-strip">
@@ -375,10 +372,7 @@
@if (isSyncing) {
<h1>{{ t("syncing_project") }}</h1>
@if (targetProjectDoc$ | async; as projectDoc) {
- <app-sync-progress
- [projectDoc]="$any(projectDoc)"
- (inProgress)="updateSyncStatus($event)"
- ></app-sync-progress>
+ <app-sync-progress [projectDoc]="projectDoc" (inProgress)="updateSyncStatus($event)"></app-sync-progress>
}
}
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 9 files and made 4 comments.
Reviewable status: 22 of 25 files reviewed, 15 unresolved discussions (waiting on @pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 336 at r4 (raw file):
I'm a bit confused about the meaning of the first "with Paratext" in
with Paratext and synced with Paratext.
Can you explain what that is implying? I wonder if that was a mistake and we should delete those two words?
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 350 at r4 (raw file):
wizard
I expect many users will not be familiar with the term "wizard" for a multi-step dialog. Maybe this should say
Please close the dialog and try again.
I see that Microsoft's style guide, for example, discourages using the term "wizard".
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 147 at r4 (raw file):
projectLoadingFailed = false; sourceProjectId?: string; noDraftsAvailable = false;
This variable is never set to true. Should we remove it?
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 353 at r4 (raw file):
"ready_to_import": "Ready to import draft to “{{ projectShortName }} - {{ projectName }}”", "retry": "Retry", "setting_up_project": "Setting up {{ projectName }}...",
I suggest putting a space before the ellipsis. I looked to see what the Microsoft Style Guide would say about this, and found that they discourage use of ellipsis anyway. I won't suggest that here, as I think this is a good place for it. But I do see that in their discussion and examples, they do separate an ellipsis from other text by a space.
Some additional examples can be found here as well.
What do you think about changing
-"setting_up_project": "Setting up {{ projectName }}...",
+"setting_up_project": "Setting up {{ projectName }} ...",
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@marksvc reviewed 3 files and made 2 comments.
Reviewable status: all files reviewed (commit messages unreviewed), 17 unresolved discussions (waiting on @pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard-component.spec.ts line 208 at r4 (raw file):
}); class TestEnvironment {
I've observed simplicity here that I've liked.
📌 Just saying.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard-component.spec.ts line 220 at r4 (raw file):
when(mockActivatedProjectService.projectId).thenReturn('project01'); when(mockParatextService.getProjects()).thenResolve([ // Source project
Nice neatly defined and commented projects.
📌 Just saying.
pmachapman
force-pushed
the
feature/SF-3633
branch
from
cca7995 to
6b47d07
Compare
pmachapman
left a comment
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman made 13 comments and resolved 11 discussions.
Reviewable status: all files reviewed (commit messages unreviewed), 6 unresolved discussions (waiting on @marksvc).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 57 at r1 (raw file):
Previously, marksvc wrote…
I did some investigation and experimenting regarding these
$any(which I perceive as a type-unsafe cast).It looks like because
(targetProject$ | async); as projectis in the@if (...), we won't do the condition if(targetProject$ | async)is not truthy. And so ifprojectis undefined, then we don't go into the if block and useproject.It looks like we can simply remove the
$anycasts. For example, the following diff.Though maybe the $any is doing something else that I am overlooking?
@@ -50,7 +50,7 @@ @if (projectSelectionForm.valid && !isLoadingProject && (targetProject$ | async); as project) { <app-notice [icon]="'verified'"> - {{ t("ready_to_import", { projectShortName: $any(project).shortName, projectName: $any(project).name }) }} + {{ t("ready_to_import", { projectShortName: project.shortName, projectName: project.name }) }} </app-notice> } @@ -130,10 +130,7 @@ @if (isConnecting) { <h1>{{ t("connecting_to_project") }}</h1> @if (targetProjectDoc$ | async; as projectDoc) { - <app-sync-progress - [projectDoc]="$any(projectDoc)" - (inProgress)="updateConnectStatus($event)" - ></app-sync-progress> + <app-sync-progress [projectDoc]="projectDoc" (inProgress)="updateConnectStatus($event)"></app-sync-progress> } @else { <p>{{ t("setting_up_project", { projectName: selectedParatextProject?.name }) }}</p> <mat-progress-bar mode="indeterminate" color="accent"></mat-progress-bar> @@ -157,7 +154,7 @@ <h1>{{ t("connected_to_project") }}</h1> @if (targetProject$ | async; as project) { <app-notice [icon]="'verified'"> - {{ t("ready_to_import", { projectShortName: $any(project).shortName, projectName: $any(project).name }) }} + {{ t("ready_to_import", { projectShortName: project.shortName, projectName: project.name }) }} </app-notice> } <div class="button-strip"> @@ -375,10 +372,7 @@ @if (isSyncing) { <h1>{{ t("syncing_project") }}</h1> @if (targetProjectDoc$ | async; as projectDoc) { - <app-sync-progress - [projectDoc]="$any(projectDoc)" - (inProgress)="updateSyncStatus($event)" - ></app-sync-progress> + <app-sync-progress [projectDoc]="projectDoc" (inProgress)="updateSyncStatus($event)"></app-sync-progress> } }
Done. I think you are right, and my quick testing confirmed it. I have removed any$ where it was used.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 170 at r1 (raw file):
Previously, marksvc wrote…
(Just saying. Converting to 'Discussing' so it's seen.)
Yes. This is part of why I couldn't figure out a way to move away from the stepper without a complete rewrite :-(
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 217 at r1 (raw file):
Previously, marksvc wrote…
(Just saying. Converting to 'Discussing' so it's seen.)
Done. I thought this was necessary for a scenario where the user was a non-PT user for a project, but I have just tested this and found that a project won't even show up in this case. I have removed targetProject$.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 233 at r1 (raw file):
Previously, marksvc wrote…
(Just saying. Converting to 'Discussing' so it's seen.)
I guess I see the event as more of a system generated event (i.e. sync completed), rather than a user-generated event (i.e. a button click)?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 490 at r2 (raw file):
Previously, marksvc wrote…
(Just saying. Converting to 'Discussing' so it's seen.)
Done. I have removed this logic as it is a hangover from previous logic that has been rewritten. Thank you for questioning things like this!
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 492 at r2 (raw file):
Previously, marksvc wrote…
(Just saying. Converting to 'Discussing' so it's seen.)
Done. I've cleaned up this method a bit to match other changes in the code.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 498 at r2 (raw file):
Previously, marksvc wrote…
(Just saying. Converting to 'Discussing' so it's seen.)
This will now only occur on the first step, when the user attempts to select a project that is already in SF that they do not have write access to.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 515 at r2 (raw file):
Previously, marksvc wrote…
(Just saying. Converting to 'Discussing' so it's seen.)
ensureSelectedBooksExist has been rewritten to be ensureProjectExists.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 147 at r4 (raw file):
Previously, marksvc wrote…
This variable is never set to true. Should we remove it?
Done. Removed - thank you for spotting this.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 611 at r4 (raw file):
Previously, marksvc wrote…
I wonder what the user sees when this happens, as here we just
returnfrom the method without seeming to tell the user anything.📌 Just saying.
I don't think this if block's scenario can be hit, but I have updated this to set this.cannotAdvanceFromProjectSelection = true;, which will show an appropriate error to the user.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 336 at r4 (raw file):
Previously, marksvc wrote…
I'm a bit confused about the meaning of the first "with Paratext" in
with Paratext and synced with Paratext.
Can you explain what that is implying? I wonder if that was a mistake and we should delete those two words?
Done. I think it is a wording error - fixed.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 350 at r4 (raw file):
Previously, marksvc wrote…
wizard
I expect many users will not be familiar with the term "wizard" for a multi-step dialog. Maybe this should say
Please close the dialog and try again.
I see that Microsoft's style guide, for example, discourages using the term "wizard".
Done. Good thinking.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 353 at r4 (raw file):
Previously, marksvc wrote…
I suggest putting a space before the ellipsis. I looked to see what the Microsoft Style Guide would say about this, and found that they discourage use of ellipsis anyway. I won't suggest that here, as I think this is a good place for it. But I do see that in their discussion and examples, they do separate an ellipsis from other text by a space.
Some additional examples can be found here as well.
What do you think about changing
-"setting_up_project": "Setting up {{ projectName }}...", +"setting_up_project": "Setting up {{ projectName }} ...",
Done. Thank you!
marksvc
left a comment
marksvc
left a comment
There was a problem hiding this comment.
@marksvc partially reviewed 13 files, made 2 comments, and resolved 6 discussions.
Reviewable status: 27 of 35 files reviewed, 1 unresolved discussion (waiting on @pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.ts line 217 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Done. I thought this was necessary for a scenario where the user was a non-PT user for a project, but I have just tested this and found that a project won't even show up in this case. I have removed
targetProject$.
:-O Oh, okay, good find.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 302 at r5 (raw file):
<div class="failed-chapters"> <mat-icon class="error-icon">error</mat-icon> <span>{{ t("failed") }} {{ getFailedChapters(progress) }}</span>
Good catch :)
📌
marksvc
added
ready to test
and removed
will require testing
pmachapman
left a comment
pmachapman
left a comment
There was a problem hiding this comment.
@pmachapman resolved 1 discussion.
Reviewable status: 27 of 35 files reviewed, all discussions resolved (waiting on @marksvc).
pmachapman
force-pushed
the
feature/SF-3633
branch
from
6b47d07 to
e61ce3c
Compare
pmachapman
force-pushed
the
feature/SF-3633
branch
from
b6e26d9 to
903f404
Compare
pmachapman
force-pushed
the
feature/SF-3633
branch
from
903f404 to
a51eb55
Compare
pmachapman
force-pushed
the
feature/SF-3633
branch
from
a51eb55 to
0158fde
Compare
2 participants
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