SF-3007 Add audio comments

[RaymondLuong3]

This PR allows users to add audio to their comments. This re-uses the TextAndAudio component and replaces the old comment input with this component. Audio can be played back to users in the form of the single button audio player.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 83.14607% with 15 lines in your changes missing coverage. Please review.

Project coverage is 79.21%. Comparing base (f132e3d) to head (c8bdb59).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...pp/src/app/checking/checking/checking.component.ts	76.19%	5 Missing ⚠️
...ecking-input-form/checking-input-form.component.ts	92.59%	1 Missing and 3 partials ⚠️
...pture/ClientApp/src/app/checking/checking.utils.ts	25.00%	2 Missing and 1 partial ⚠️
...dio-player/single-button-audio-player.component.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2815      +/-   ##
==========================================
- Coverage   79.21%   79.21%   -0.01%     
==========================================
  Files         533      533              
  Lines       30947    30974      +27     
  Branches     5025     5031       +6     
==========================================
+ Hits        24515    24536      +21     
- Misses       5659     5666       +7     
+ Partials      773      772       -1     

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

[kylebuss]

This is looking good Raymond. Just one small suggestion.

Reviewed 16 of 16 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 17 at r2 (raw file):

            >
              <mat-icon>{{ audio.playing ? "stop" : "play_arrow" }}</mat-icon>
            </app-single-button-audio-player>

I can't decide if I like the single button player or if it should use the app-checking-audio-player. If we use the single button player and no text is entered I think we should have a default "Listen to the comment" similar what we do for a question.

Code quote:

            <app-single-button-audio-player
              #audio
              [source]="comment.audioUrl"
              class="comment-audio"
              (click)="audio.play()"
            >
              <mat-icon>{{ audio.playing ? "stop" : "play_arrow" }}</mat-icon>
            </app-single-button-audio-player>

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.ts line 198 at r2 (raw file):

      action: 'save',
      answer: this.answer,
      text: comment.text,

See note above about adding a default comment when no text is present.

Code quote:

text: comment.text

[RaymondLuong3]

Reviewable status: 15 of 18 files reviewed, 1 unresolved discussion (waiting on @kylebuss)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 17 at r2 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

I can't decide if I like the single button player or if it should use the app-checking-audio-player. If we use the single button player and no text is entered I think we should have a default "Listen to the comment" similar what we do for a question.

I like the suggestion for a placeholder text. How about this?

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.ts line 198 at r2 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

See note above about adding a default comment when no text is present.

Right, I added a placeholder text in the template. We don't want to add data directly to the objects.

[kylebuss]

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

[kylebuss]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

[josephmyers]

Good job adding the base feature. But there are a number of things missing from the wireframe, and we should implement those here, unless you have a specific for not doing so. I'll try to list them all here, but be sure to look there for yourself. Keep in mind the biggest consideration for Community Checking has to be checkers on mobile. Yes, admins on desktop are important, too, but we should be fitting their additional features around the core checking experience.

• For answers, Edit/Delete should be wrapped up into icons and placed on the same row as the avatar and date. For admin buttons, we could include them on that row, too, probably as icons. We could also only use icons on mobile (when space is limited) -- but it'd be simpler to just use icons (with tooltips).
• For comments, the name and date should display on the right (two rows), with Edit/Delete appearing to the left of them. For audio only, we can display all this on the same line as the audio player. When there's text, we should pop all this down to one row, below text and any audio.
• As part of the above work, I also recommend removing the placeholder text. That space is too valuable, and I think it would be confusing.
• Make sure the content is vertically centered. Currently, when the name and date take up two rows and the content is only one, the content is at the top, and it looks a little weird.
• Consider replacing the divider lines with alternating backgrounds. This would help us continue to move toward a more modern UX, if we could get it to look good.
• Any reason for not adding the comment icon from the design?
  

Also

• How difficult would it be to add a loading symbol while the audio is being uploaded? The interim between submission and display is noticeable, and it looks like a bug until it appears.
• You can't stop the audio once it starts playing. Tapping the stop button does nothing.

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @RaymondLuong3)

[RaymondLuong3]

Here are the updated images

Comment no actions

Audio comment with actions

Audio comment full screen

[RaymondLuong3]

As for adding a loading symbol when comments are being uploaded, I think we should create a new issue for this since it affects questions, answers and comments. If we add a loading symbol it would make sense to add them in all 3 scenarios.

Thanks for the feedback, I didn't realize the balsamiq mock-up was a larger redesign and just implemented the audio comment part, but I really like the redesign and the update to using icons.

Reviewable status: 14 of 22 files reviewed, all discussions resolved (waiting on @kylebuss)

[josephmyers]

Cool, thanks for that turnaround time! I'm going to get a little picky with some of these comments, but it's important for this with how limited the space is.

• The Add Comment button hover is touching the divider
  
• Did you try removing the dividers in favor of the alternating (very light) background? Did it look bad?
• The Edit and Delete icons should be smaller. They should appear smaller than the audio player
• The Export and Archive buttons on the answers should probably be icons on mobile, at least. Right now, they're on their own row, and it looks a little strange.
• The author and date should be on top of one another, like they are for answers. They should all align on the right, which will help with the clarity of this section.
• Somewhat related, I don't think the author should be styled the exact same as the content itself. We could do better to break up the sameness
• When the text is small, Edit and Delete appear on the same line as it. The only time these two should not be in the bottom left is for audio only.
  

This is all I have seen so far. Thanks for hanging in there

Reviewed 1 of 8 files at r5, all commit messages.
Reviewable status: 15 of 22 files reviewed, 1 unresolved discussion (waiting on @kylebuss and @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 58 at r6 (raw file):

      }
      @if (commentFormVisible && activeComment?.dataId === comment.dataId) {
        <app-checking-comment-form

Nathaniel made the good point that this component (form) is really similar to the answers form. We should consider reusing this form for the answers. Obviously there will be some minor differences, but we can pass those variables in as parameters. Some things can just align, e.g. "Save comment" can become "Save"

Here are some things I notice that will need to be parameterized:

• Attach verses, enabled/disabled
• Input box height
• Input box default text

I'm open to discussion on this at a conceptual level. There is a risk that there will be enough differences in the future that we overcomplicate things by trying to share this code. However, I lean more toward sharing, as I don't foresee many differences upcoming.

[RaymondLuong3]

Thanks for the feedback. I believe I have addressed most of the comments. I added the alternating background between comments, I'm not entirely sold but it works.
The flag for review and resolve buttons I left as is. Since this is only visible to admins, I didn't want to do too much. Their role is fundamentally different from the edit/delete button and I thought it would warrant being text buttons rather than icon buttons. Plus, there background colour when they are selected doesn't look very good if converted to icon buttons.
I didn't update the author styling. We can do that in a follow up if desired.

I also took the time to update the answer form to use the comment form. I like that we can pull combine the behaviour of the forms into one form. I like the result but we should be careful to do testing to ensure all features continue to work. I'll comment on the Jira card to look for regressions.

Reviewable status: 9 of 29 files reviewed, 1 unresolved discussion (waiting on @josephmyers and @kylebuss)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 58 at r6 (raw file):

Previously, josephmyers wrote…

Nathaniel made the good point that this component (form) is really similar to the answers form. We should consider reusing this form for the answers. Obviously there will be some minor differences, but we can pass those variables in as parameters. Some things can just align, e.g. "Save comment" can become "Save"

Here are some things I notice that will need to be parameterized:

• Attach verses, enabled/disabled
• Input box height
• Input box default text

I'm open to discussion on this at a conceptual level. There is a risk that there will be enough differences in the future that we overcomplicate things by trying to share this code. However, I lean more toward sharing, as I don't foresee many differences upcoming.

Yep, it was definitely not trivial, but in the long term I think combining the forms was worth the effort.

[josephmyers]

Cool, man, thanks for your work on this. It's getting there! Some general comments:

• We really should be styling the author name for this. Just take the author's font size down a smidge. Try using the same font size as the date, but not changing anything else.
• Author name should be right aligned.
• Rename the "comment form" component + files to not specify comments anymore.
• Sometimes the alternating background doesn't appear. Really weird

Screenshot of mobile as an observer:

Reviewed 2 of 19 files at r7, all commit messages.
Reviewable status: 11 of 29 files reviewed, 11 unresolved discussions (waiting on @kylebuss and @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-global-vars.scss line 15 at r7 (raw file):

$borderColor: rgba(0, 0, 0, 0.1);
$notifyUser: $errorColor;
$shadedBackgroundColor: $borderColor;

The alternating background should not be the same color as the border. It needs to be lighter, aesthetically and for the theoretical use case where it's used in conjunction with borders. How about alpha 0.05?

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.html line 52 at r7 (raw file):

      } @else {
        <div id="answer-form">
          <app-checking-comment-form

I get this when tapping Add Answer (I'm on Dev Tools mobile)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.scss line 1 at r7 (raw file):

@use 'src/variables';

In this file is a border-bottom rule which adds a divider between the answer and any comments. We should remove this. Then the only place that uses dividers will be between answers

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.html line 7 at r7 (raw file):

      appAutofocus
      [input]="textAndAudioInput"
      [textLabel]="t('your_comment')"

Says Your Comment for answers. Needs to be parameterized

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.ts line 75 at r7 (raw file):

    this.selectedText = value?.scriptureText;
    this.verseRef = value?.verseRef;
    this.compact = false;

This property should be an Input, and it should be specified/set in the HTML, not another property's setter

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.ts line 80 at r7 (raw file):

  @Input() set comment(value: Comment | undefined) {
    this.textAndAudioInput = value;

It would be much better to be combine the comment and answer Inputs, if possible. This should be, given that they share a type. Could call it "input"

Similarly, the selectedText and verseRef are totally unlinked from their parent property, textSelectionEnabled. When you combine the two Inputs, there should be some check to confirm that selectedText and verseRef are present if the textSelectedEnabled is true.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.ts line 93 at r7 (raw file):

  selectScripture(): void {
    if (this.textsByBookId == null || this._questionDoc?.data == null) return;

This method should likely be gated behind the textSelectionEnabled you made. Same for the rest of the file

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r7 (raw file):

        (i + 1 < maxCommentsToShow || commentCount === maxCommentsToShow || showAllComments)
      ) {
        <div class="comment" [ngClass]="{ 'comment-unread': !hasUserReadComment(comment), shaded: i % 2 !== 0 }">

This is great, but can we experiment with adding the condition that we only shade when there are >2 comments shown? Specifically shown, since we also have a behavior to collapse when there are many comments

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 20 at r7 (raw file):

          }
          @if (comment.text != null) {
            <span class="comment-text">{{ comment.text }}</span>

At some point since I last looked, when there are both text and audio, text is now put on the next line. They should both be on the same line, but keep in mind that the text can wrap.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 40 at r7 (raw file):

    .mat-icon {
      transform: scale(0.8);

I recommend not using scale here. It makes it harder to control and know what the actual value is. You should be able to use line-height or font-size (or both) to set the icon size. I believe the smallest icons we're using are 18px.

Similarly, please make the icons for the answer the same size as the comments. Right now they're different

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 41 at r7 (raw file):

    .mat-icon {
      transform: scale(0.8);
    }

Another problem with scale is that it makes the layout harder to set. In the pic above, the spacing is all weird, especially for the audio only comment. The spacing between the two icons should be smaller than between them and the audio player. This will be critical in helping users grasp that there's a difference between the icons and the audio.

On a related note, it would be nice, though optional, if the Edit icon for the previous comment were centered over the audio player. That's a little picky, though

[RaymondLuong3]

Thanks, I've updated this with your suggested changes. I did not see the alternative background not working issue that you saw.

Reviewable status: 7 of 30 files reviewed, 11 unresolved discussions (waiting on @josephmyers and @kylebuss)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-global-vars.scss line 15 at r7 (raw file):

Previously, josephmyers wrote…

The alternating background should not be the same color as the border. It needs to be lighter, aesthetically and for the theoretical use case where it's used in conjunction with borders. How about alpha 0.05?

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.html line 52 at r7 (raw file):

Previously, josephmyers wrote…

I get this when tapping Add Answer (I'm on Dev Tools mobile)

This was a tricky one that I tracked down to the breakpointObserver. I couldn't figure out how to prevent it other than by delaying the breakpoint subscriber by a clock cycle.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.scss line 1 at r7 (raw file):

Previously, josephmyers wrote…

In this file is a border-bottom rule which adds a divider between the answer and any comments. We should remove this. Then the only place that uses dividers will be between answers

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r7 (raw file):

Previously, josephmyers wrote…

This is great, but can we experiment with adding the condition that we only shade when there are >2 comments shown? Specifically shown, since we also have a behavior to collapse when there are many comments

Yes, that is possible. I have updated it accordingly to behave as you described

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 20 at r7 (raw file):

Previously, josephmyers wrote…

At some point since I last looked, when there are both text and audio, text is now put on the next line. They should both be on the same line, but keep in mind that the text can wrap.

Good catch. Fixed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 40 at r7 (raw file):

Previously, josephmyers wrote…

I recommend not using scale here. It makes it harder to control and know what the actual value is. You should be able to use line-height or font-size (or both) to set the icon size. I believe the smallest icons we're using are 18px.

Similarly, please make the icons for the answer the same size as the comments. Right now they're different

I've added a mixin that will get the icon to appear smaller.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 41 at r7 (raw file):

Previously, josephmyers wrote…

Another problem with scale is that it makes the layout harder to set. In the pic above, the spacing is all weird, especially for the audio only comment. The spacing between the two icons should be smaller than between them and the audio player. This will be critical in helping users grasp that there's a difference between the icons and the audio.

On a related note, it would be nice, though optional, if the Edit icon for the previous comment were centered over the audio player. That's a little picky, though

I have increased the spacing between the play button and the action buttons to 8px. Let me know if you think more is needed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.html line 7 at r7 (raw file):

Previously, josephmyers wrote…

Says Your Comment for answers. Needs to be parameterized

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.ts line 75 at r7 (raw file):

Previously, josephmyers wrote…

This property should be an Input, and it should be specified/set in the HTML, not another property's setter

I've removed this and just styled in from the parent CheckingCommentComponent. Unfortunately something isn't working when there is no existing comment, so on new comments the input opens up at a larger height. I have run out of ideas on what to try.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.ts line 80 at r7 (raw file):

Previously, josephmyers wrote…

It would be much better to be combine the comment and answer Inputs, if possible. This should be, given that they share a type. Could call it "input"

Similarly, the selectedText and verseRef are totally unlinked from their parent property, textSelectionEnabled. When you combine the two Inputs, there should be some check to confirm that selectedText and verseRef are present if the textSelectedEnabled is true.

Combining the two inputs into one seems to be a good thing. I've named that input checkingInput since the component has been renamed CheckingInputForm

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.ts line 93 at r7 (raw file):

Previously, josephmyers wrote…

This method should likely be gated behind the textSelectionEnabled you made. Same for the rest of the file

Done.

[josephmyers]

Looking really good. Here are some minor comments:

• The icons are very black and bold, and they really catch the eye (mostly Admin view). The default alpha there currently is 0.87 -- can you tweak this down to 0.70 and see what it looks like?
• The author is not fully right aligned for comments. It doesn't match the date below or the answer above.

Reviewed 6 of 19 files at r8, all commit messages.
Reviewable status: 13 of 30 files reviewed, 6 unresolved discussions (waiting on @kylebuss and @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.html line 52 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

This was a tricky one that I tracked down to the breakpointObserver. I couldn't figure out how to prevent it other than by delaying the breakpoint subscriber by a clock cycle.

Just move the whole .subscribe() into the constructor. No need for AfterViewInit

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.scss line 1 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Done.

Looks great, thanks

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Yes, that is possible. I have updated it accordingly to behave as you described

Thanks for this. I think the problem was that the first comment always looked weird. I've been playing around with shaded: i % 2 === 0 and I think it looks a lot better. No alternateBackground property needed

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 40 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I've added a mixin that will get the icon to appear smaller.

Great idea. Any reason you didn't use 18px? The Add Comment icon you added is 18px, but these are 20px. The former has an applied style from being in a button, but the other icons should match this when possible.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 41 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I have increased the spacing between the play button and the action buttons to 8px. Let me know if you think more is needed.

It's good to increase that, thanks. But the core problem is still there, since the buttons that contain the icons are the same size as before. The Edit icon is still closer to the audio than to the Trash icon.

I'm assuming you didn't change the buttons themselves because of the Material guidelines for clickable areas. 40px is the lowest we typically go. I don't like the idea of increasing the Audio<->Edit spacing even more, but we could consider that.

I think you've exposed a small area where the Balsamiq doesn't quite work. Another design to try is to bump Edit/Delete down to the second row always. (This will result in the top row being pretty sparse for audio only, but the icons won't conflict for spacing, and it'll be more uniform.) Let's try this and see if it works

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.ts line 75 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I've removed this and just styled in from the parent CheckingCommentComponent. Unfortunately something isn't working when there is no existing comment, so on new comments the input opens up at a larger height. I have run out of ideas on what to try.

I'm not sure why you removed the property altogether, instead of making it an Input. Maybe I wasn't clear enough.

If making compact an Input doesn't work for you, you could just base the style off of the label you just made. Setting the compact class on the app-text-and-audio, like you had before, but something like this:

Code snippet:

[ngClass]="{ compact: label === 'comment' }"

[RaymondLuong3]

Reviewable status: 13 of 30 files reviewed, 6 unresolved discussions (waiting on @josephmyers and @kylebuss)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.html line 52 at r7 (raw file):

Previously, josephmyers wrote…

Just move the whole .subscribe() into the constructor. No need for AfterViewInit

That's it! Thanks.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r7 (raw file):

Previously, josephmyers wrote…

Thanks for this. I think the problem was that the first comment always looked weird. I've been playing around with shaded: i % 2 === 0 and I think it looks a lot better. No alternateBackground property needed

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 40 at r7 (raw file):

Previously, josephmyers wrote…

Great idea. Any reason you didn't use 18px? The Add Comment icon you added is 18px, but these are 20px. The former has an applied style from being in a button, but the other icons should match this when possible.

I moved it down to 18px and it is starting to look tiny on mobile, but it will be easy to adjust bigger if it is needed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 41 at r7 (raw file):

Previously, josephmyers wrote…

It's good to increase that, thanks. But the core problem is still there, since the buttons that contain the icons are the same size as before. The Edit icon is still closer to the audio than to the Trash icon.

I'm assuming you didn't change the buttons themselves because of the Material guidelines for clickable areas. 40px is the lowest we typically go. I don't like the idea of increasing the Audio<->Edit spacing even more, but we could consider that.

I think you've exposed a small area where the Balsamiq doesn't quite work. Another design to try is to bump Edit/Delete down to the second row always. (This will result in the top row being pretty sparse for audio only, but the icons won't conflict for spacing, and it'll be more uniform.) Let's try this and see if it works

I think moving the action buttons onto a new like is the right way to go. All of the delete buttons will be aligned when admins are looking at it. For community checkers, the icon alignment is not as big a deal unless they have left lots of comments in the thread.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comment-form/checking-comment-form.component.ts line 75 at r7 (raw file):

Previously, josephmyers wrote…

I'm not sure why you removed the property altogether, instead of making it an Input. Maybe I wasn't clear enough.

If making compact an Input doesn't work for you, you could just base the style off of the label you just made. Setting the compact class on the app-text-and-audio, like you had before, but something like this:

Done.

[josephmyers]

Few more small comments. But, man, this is starting to look really good, especially for community checkers on mobile

Reviewed 1 of 8 files at r5, 3 of 19 files at r7, 6 of 19 files at r8, 10 of 10 files at r9, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts line 1244 at r9 (raw file):

      }
    }
    return true;

This is a little weird to me. If audio == null, we return true?

I'm assuming this is to account for audio simply not being present on submission. If that's the case, this method and its usages would be much, much clearer if we extracted the wrapping prerequisite checks, e.g. audio == null, out to the callers. In effect, we wouldn't call this unless the submission has audio.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Done.

Your code says !==. It should be ===, unless you specifically intended this. In my opinion, the first comment looks pretty slick being gray, whereas having the second gray looks off.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r9 (raw file):

        (i + 1 < maxCommentsToShow || commentCount === maxCommentsToShow || showAllComments)
      ) {
        <div class="comment" [ngClass]="{ 'comment-unread': !hasUserReadComment(comment), shaded: i % 2 !== 0 }">

I am still seeing a tiny bit of flakiness with the alternating background. It's really weird

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 56 at r9 (raw file):

      }
      @if (commentFormVisible && activeComment?.dataId === comment.dataId) {
        <app-checking-input-form

We need a tiny bit of top margin/padding for when the textarea pushes the placeholder up. Around 4px. And check the other usages of app-checking-input-form, too. Thanks

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 40 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I moved it down to 18px and it is starting to look tiny on mobile, but it will be easy to adjust bigger if it is needed.

Sounds good, thanks for being flexible. I'm fine either way, I just want to be consistent

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.scss line 41 at r7 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I think moving the action buttons onto a new like is the right way to go. All of the delete buttons will be aligned when admins are looking at it. For community checkers, the icon alignment is not as big a deal unless they have left lots of comments in the thread.

Yeah, agreed. This is a good change

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/audio-recorder-dialog/audio-recorder-dialog.component.spec.ts line 64 at r9 (raw file):

  it('can record', async () => {
    const env = new TestEnvironment();
    env.waitForRecorder(300);

Is this something we still need?

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.html line 4 at r9 (raw file):

  <form autocomplete="off" appScrollIntoView>
    <app-text-and-audio
      #commentInput

Still a few places in this file whose names mention "comment" when they shouldn't

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.html line 12 at r9 (raw file):

    ></app-text-and-audio>
    <div class="form-actions">
      <div class="flex-row">

Now, the Record and Attach buttons have a gap between them. I believe it's coming from the flex-row

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.scss line 7 at r9 (raw file):

  align-items: center;

  .button-actions {

Unused

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.ts line 55 at r9 (raw file):

  isScreenSmall: boolean = false;
  submittingResponse: boolean = false;
  verseRef?: VerseRefData;

Make this private, and use it directly with this. in the method that needs it

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.ts line 56 at r9 (raw file):

  submittingResponse: boolean = false;
  verseRef?: VerseRefData;
  textAndAudioInput?: { text?: string; audioUrl?: string };

Please make this private, with a public or protected access method/getter to expose it

[RaymondLuong3]

Reviewable status: 23 of 30 files reviewed, 11 unresolved discussions (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts line 1244 at r9 (raw file):

Previously, josephmyers wrote…

This is a little weird to me. If audio == null, we return true?

I'm assuming this is to account for audio simply not being present on submission. If that's the case, this method and its usages would be much, much clearer if we extracted the wrapping prerequisite checks, e.g. audio == null, out to the callers. In effect, we wouldn't call this unless the submission has audio.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r7 (raw file):

Previously, josephmyers wrote…

Your code says !==. It should be ===, unless you specifically intended this. In my opinion, the first comment looks pretty slick being gray, whereas having the second gray looks off.

Oops, I see what you mean now. Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r9 (raw file):

Previously, josephmyers wrote…

I am still seeing a tiny bit of flakiness with the alternating background. It's really weird

I'm not able to reproduce this. Perhaps we can open another issue to investigate this?

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 56 at r9 (raw file):

Previously, josephmyers wrote…

We need a tiny bit of top margin/padding for when the textarea pushes the placeholder up. Around 4px. And check the other usages of app-checking-input-form, too. Thanks

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/audio-recorder-dialog/audio-recorder-dialog.component.spec.ts line 64 at r9 (raw file):

Previously, josephmyers wrote…

Is this something we still need?

It doesn't seem so. Removed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.html line 4 at r9 (raw file):

Previously, josephmyers wrote…

Still a few places in this file whose names mention "comment" when they shouldn't

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.html line 12 at r9 (raw file):

Previously, josephmyers wrote…

Now, the Record and Attach buttons have a gap between them. I believe it's coming from the flex-row

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.scss line 7 at r9 (raw file):

Previously, josephmyers wrote…

Unused

Good catch. Removed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.ts line 55 at r9 (raw file):

Previously, josephmyers wrote…

Make this private, and use it directly with this. in the method that needs it

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.ts line 56 at r9 (raw file):

Previously, josephmyers wrote…

Please make this private, with a public or protected access method/getter to expose it

Done.

[RaymondLuong3]

Reviewable status: 23 of 30 files reviewed, 11 unresolved discussions (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/audio-recorder-dialog/audio-recorder-dialog.component.spec.ts line 64 at r9 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

It doesn't seem so. Removed.

Oh, seems like this test is flaky on the CI build. Let's exclude it and come back to it.

[josephmyers]

One minor comment, that doesn't block sending to test. Great job!

Reviewed 5 of 7 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 8 at r9 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I'm not able to reproduce this. Perhaps we can open another issue to investigate this?

Sure, just be aware of it. I've seen it a couple times

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-comments/checking-comments.component.html line 56 at r9 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Done.

At first, I really didn't like this. Hard spacing below items is not a good way to add spacing intended to spread them out. You typically want to use gap properties on the container. But in this case, I think it's fine. The spacing here helps everything that can be below the last comment to breath better. It's not super clear, but it does work as is.

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/audio-recorder-dialog/audio-recorder-dialog.component.spec.ts line 64 at r9 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Oh, seems like this test is flaky on the CI build. Let's exclude it and come back to it.

If these are the two options, I'd prefer we have the 300ms wait and keep it running. It's better to run slowly than not at all

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-input-form/checking-input-form.component.html line 12 at r9 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Done.

Great, thanks

[josephmyers]

@RaymondLuong3 I thought about holding this up until you've rebased, but maybe we can get away without it. You'll want to do this soon though. It'll be cool to see this new code with the new recording code.

[josephmyers]

On this branch, I'm seeing the "scroll into view" behavior broken for comments, particularly ones toward the bottom of the screen. This would be a regression, though I'm not quite sure how this caused it. I originally thought you were missing the fix I made a week or two ago, but that doesn't appear to be the case. You may be interested to investigate a little more as to why it's being weird now.

Given that the "scroll into view" works initially -- if you watch closely -- before the splitter resizes and scrolls it offscreen, I think there's a conflict between "scroll into view" and the splitter resize, calculateScriptureSliderPosition. I'm not entirely sure why your branch breaks it, but it almost looks like "scroll into view" is being called earlier now. Because we've messed with the component/markdown structure? One option I've found to work reliably is to delay the ScrollIntoViewDirective directive, i.e. increase the setTimeout delay from 0 to 400. But it'd be far better to fix the root problem.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @RaymondLuong3)

[RaymondLuong3]

@josephmyers I have rebased these changes onto master. I'm not sure which PR you mentioned for fixing the scroll into view. Was it PR #2596 ? I can sometimes see the comments not displaying completely, but the verse for a question always scrolls into view which is what that PR was supposed to fix. Maybe we could schedule a call to chat about this. Can you test this branch as it currently is to see if it still behaves oddly?

I can't reproduce the issues Joseph is mentioning, and this has passed testing, so I'm merging