-
-
Notifications
You must be signed in to change notification settings - Fork 5
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-2965 Filter for connected projects in the draft apply dialog #2714
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2714 +/- ##
==========================================
+ Coverage 79.06% 79.09% +0.02%
==========================================
Files 531 531
Lines 30793 30831 +38
Branches 5028 5014 -14
==========================================
+ Hits 24347 24385 +38
- Misses 5661 5672 +11
+ Partials 785 774 -11 ☔ View full report in Codecov by Sentry. |
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: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
line 96 at r1 (raw file):
.getProjects() .then(projects => { this._projects = projects?.filter(p => p.isConnected).sort(compareProjectsForSorting);
This should not be using the ParatextService, it should be getting the list of projects from the user's profile. The easiest way to do this is using SFUserProjectsService
dfeb401
to
6ccd5a6
Compare
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.
Here is how the project dialog looks with the updated title and the add button always enabled.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @Nateowami)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
line 96 at r1 (raw file):
Previously, Nateowami (Nathaniel Paulus) wrote…
This should not be using the ParatextService, it should be getting the list of projects from the user's profile. The easiest way to do this is using
SFUserProjectsService
Yes, I see that now. It simplifies several things which I am glad to see.
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.html
line 53 at r2 (raw file):
<div mat-dialog-actions> <button mat-button class="cancel-button" [mat-dialog-close]="false">{{ t("cancel") }}</button> <button mat-raised-button class="add-button" color="primary" (click)="addToProject()">
With the exception of the "Refresh" button when there's a new release, this is the only elevated button in the entire application. Perhaps we should change the default style we use, but unless/until we do so, I prefer consistency.
6ccd5a6
to
0a7ecf5
Compare
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: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @Nateowami)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.html
line 53 at r2 (raw file):
Previously, Nateowami (Nathaniel Paulus) wrote…
With the exception of the "Refresh" button when there's a new release, this is the only elevated button in the entire application. Perhaps we should change the default style we use, but unless/until we do so, I prefer consistency.
Thanks, I'm not sure how I chose the wrong button style.
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.
This is looking better.
When an error is shown, the field should be in an error state. Maybe it's hard to do that due to it being a custom form control? I'm sure it's possible, but I don't want too much effort put into it.
Notice how in the invitaiton form, the inputs themselves are in an error state:
Clicking the "add to project" button should also trigger an error state (right now it does nothing). There should be a message saying you need to select a project.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 138 at r3 (raw file):
"add_to_project": "Add to project", "book_is_empty": "{{ bookName }} in {{ projectName }} project is empty", "book_does_not_exist": "{{ bookName }} does not exist in the selected project",
Can you add a period at the end? Not everything needs to be a full sentence, but I think this (and the "please confirm" message, and "Connect it on the projects page first") should.
0a7ecf5
to
b496f69
Compare
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.
With how our project select is implemented, I was able to get the error state you were looking for with a little bit of manipulation of the component. The project select is used in other places where a blank entry is valid, so I had to work around that constraint. Let me know what you think.
Reviewable status: 0 of 8 files reviewed, 1 unresolved discussion (waiting on @Nateowami)
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 138 at r3 (raw file):
Previously, Nateowami (Nathaniel Paulus) wrote…
Can you add a period at the end? Not everything needs to be a full sentence, but I think this (and the "please confirm" message, and "Connect it on the projects page first") should.
Done.
Thanks for your work on this. It looks good now, though I haven't reviewed the code. Since I'm at the PT summit, I'm dismissing my review so someone else can pick it up. |
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.
How hard is it to customize the error message as it is misleading in this context as the user can't attach the draft to a resource. Also, the other error messages relating to the selected project don't appear in the same way this error message does which is a shame - I totally get why they don't though. Could it be possible reconfigure it to set the error state based on the logic from the component using this selector? Think of this as any other input control in a form and it is the requirement of that form to determine what is valid and invalid rather than just the input control itself.
Something else that would be useful is a progress indicator or dialog while everything is in progress. I had to wait around 7 seconds after clicking the add button wondering what was happening. I only knew it completed because I got an error message about a chapter failed to apply. This can be a separate Jira task
Reviewed 2 of 6 files at r2, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Nateowami and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.html
line 35 at r4 (raw file):
<div class="target-project-content"> @if (targetChapters$ | async; as chapters) { <mat-icon class="no-shrink">warning</mat-icon>
It would be nice to break up the lines of text with some color to help focus on what's important. What do you think about making this a warning notice?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.scss
line 28 at r4 (raw file):
} .no-shrink {
I'd move this up to be a child of target-project-content
i.e. .target-project-content > mat-icon > mat-icon
rather than introducing a new class name
b496f69
to
257b8f7
Compare
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.
Thanks for adding the issue to have the progress shown. That will be a nice enhancement.
I've tweaked the project-select
component to accept custom validators. We can set the error state in the project select component in its parent component. It works when a user selects a project, and the error will show in under the select component. I could not get it to update if the user edits their selection manually completely right, but that is a minor thing.
Reviewable status: 1 of 10 files reviewed, 2 unresolved discussions (waiting on @Nateowami and @nigel-wells)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.html
line 35 at r4 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
It would be nice to break up the lines of text with some color to help focus on what's important. What do you think about making this a warning notice?
Yes, I like it. I've made the update.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.scss
line 28 at r4 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
I'd move this up to be a child of
target-project-content
i.e..target-project-content > mat-icon > mat-icon
rather than introducing a new class name
Done.
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.
Sorry I missed this one in my inbox.
Reviewed 9 of 9 files at r5, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Nateowami and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.html
line 13 at r5 (raw file):
/> <mat-error id="invalidSelection">{{ customMessage !== "" ? customMessage : t("please_select_valid_project_or_resource")
How about handling everything inside of customMessage
src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.ts
line 132 at r5 (raw file):
} get customMessage(): string {
Could we call this invalidMessage
or similar so its clear it relates to that state.
src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.ts
line 145 at r5 (raw file):
return translate('project_select.no_write_permissions'); } return '';
As mentioned above, just do return translate("please_select_valid_project_or_resource")
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.html
line 31 at r5 (raw file):
<div class="target-project-content"> @if (targetChapters$ | async; as chapters) { <app-notice [icon]="'warning'" [type]="'warning'"
nit to avoid the double/single quotes you can just write it as a non binding attribute i.e. icon="warning"
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 374 at r5 (raw file):
}, "project_select": { "book_does_not_exist": "The book on the selected project does not exist",
nit bonus points if you can include the name of the book in this message i.e. "Exodus does not exist on the selected project" as I think that makes it clearer. Same with the error below if you're able to do it.
257b8f7
to
cbe2903
Compare
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: 5 of 10 files reviewed, 4 unresolved discussions (waiting on @Nateowami and @nigel-wells)
src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.html
line 13 at r5 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
How about handling everything inside of
customMessage
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.ts
line 132 at r5 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
Could we call this
invalidMessage
or similar so its clear it relates to that state.
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/project-select/project-select.component.ts
line 145 at r5 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
As mentioned above, just do
return translate("please_select_valid_project_or_resource")
Done.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.html
line 31 at r5 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
nit to avoid the double/single quotes you can just write it as a non binding attribute i.e.
icon="warning"
Great! Done.
src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json
line 374 at r5 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
nit bonus points if you can include the name of the book in this message i.e. "Exodus does not exist on the selected project" as I think that makes it clearer. Same with the error below if you're able to do it.
It was a little more work, but it has it now.
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.
This is functionally doing and looking right.
I wonder if we're starting to introduce too much other logic into the project select component that is only relevant to the draft component? Is it possible for the custom validator that you're passing in to be able to handle the error message at all so that the logic can remain in the relevant component.
What do you think?
Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami)
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.
I think that is probably true. Instead of having the invalid messages calculated by the project select, I added a Input into the project select that can be used to map the invalid errors to a specific message. Let me know what you think.
Reviewable status: 5 of 10 files reviewed, 1 unresolved discussion (waiting on @Nateowami and @nigel-wells)
@RaymondLuong3 The project select is a custom form field control. In theory it should be possible to add validators without editing the component. See https://material.angular.io/guide/creating-a-custom-form-field-control |
@Nateowami Thanks for the resource. It wasn't clear to me that the project select component was implemented the way the article described. I think this PR is started to move past it's original scope. I am happy to get this in so that we can start using the |
That is a fair call @RaymondLuong3 - I have unintentional moved the scope. Did you want to add in some TODOs or create another card? |
@nigel-wells I created SF-3014 to track customizing the project select component. |
@Nateowami Since Nigel is away, can you give this PR an approval to move to testing? |
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.
Back now - just one comment that might be worth considering for translation purposes. Logically I think this is fine and we can look to improve the custom field in SF-3014
Reviewed 4 of 5 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Nateowami and @RaymondLuong3)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.html
line 22 at r8 (raw file):
@if (connectOtherProject != null) { <div class="unlisted-project-message"> <p>
It's only just dawned on me now. Is this approach going to work ok for other languages? The string will be translated correctly in crowdin but can we safely dissect it this way and know it will join up correctly? I had to do something similar in the join component with join.login_existing_account
and decided to make the whole sentence clickable and then make use of the various tags to add the style to the part that needed to look like a link. Probably worth doing here as well?
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
line 59 at r8 (raw file):
); invalidMessageMapper: { [key: string]: string } = { invalidProject: translate('draft_apply_dialog.please_select_valid_project'),
nit It is best to pass the key and translate it within the component otherwise changing languages won't update the interface. As you can't change the language once the dialog is open it isn't really a big deal.
d71686a
to
1f46e6b
Compare
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: 5 of 10 files reviewed, 1 unresolved discussion (waiting on @Nateowami and @nigel-wells)
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.html
line 22 at r8 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
It's only just dawned on me now. Is this approach going to work ok for other languages? The string will be translated correctly in crowdin but can we safely dissect it this way and know it will join up correctly? I had to do something similar in the join component with
join.login_existing_account
and decided to make the whole sentence clickable and then make use of the various tags to add the style to the part that needed to look like a link. Probably worth doing here as well?
Good thinking, I do like that solution much better, and it was not too difficult to do.
src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-apply-dialog/draft-apply-dialog.component.ts
line 59 at r8 (raw file):
Previously, nigel-wells (Nigel Wells) wrote…
nit It is best to pass the key and translate it within the component otherwise changing languages won't update the interface. As you can't change the language once the dialog is open it isn't really a big deal.
I was thinking we wouldn't need to have dynamic translations because a user cannot update their language with a dialog open. I left it as-is.
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.
Reviewed 5 of 5 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami)
1f46e6b
to
18a6aef
Compare
Something went really wrong with the Storybook workflow; it ran for an hour and a half before I cancelled it. |
18a6aef
to
058ccdb
Compare
058ccdb
to
5e56a55
Compare
<a | ||
[appRouterLink]="['projects']" | ||
(click)="close()" | ||
[innerHtml]="i18n.translateAndInsertTags('draft_apply_dialog.looking_for_unlisted_project')" |
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.
@RaymondLuong3
I have some concern here.
- I really, really do not like the use of
innerHtml
, and have been trying hard to avoid it. Angular sanitizes it, but it's still kind of ugly from a security standpoint. - It seems odd for the whole sentence to be a link.
- This string has been translated into three languages, so it will have to be re-translated, and only because of a change to the variable name.
Only connected projects should be visible on the draft apply dialog project selector. Connected projects are projects that exist on SF and the user is joined to the project. This matches the my projects page for connected projects.
This change also fixes a slight UI glitch for icons when on a mobile device.
This change is