SF-3176 Add Upload Audio to Questions

[josephmyers]

A good portion of this was restored from its last working state (last year), before we removed it, and updated. The file processing comes solely from the existence of the uploadAudioFile property. I also restored the old Firefox .ogg file workaround, though I didn't test that it's still required (yet).

The upload button must be enabled to be visible. This is controlled by a boolean Input on the control, and it's false for its usage within the answers.

Also accounting for the 'uploaded' state in text-and-audio, since beforehand it was only looking for the recorded 'processed' state.

The aesthetics come from what was selected in Balsamiq, but we can certainly adjust it, based on what the result actually looks like.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 88.23529% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.04%. Comparing base (3bb5372) to head (d148f8a).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...pp/checking/attach-audio/attach-audio.component.ts	86.66%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2984      +/-   ##
==========================================
- Coverage   82.04%   82.04%   -0.01%     
==========================================
  Files         545      545              
  Lines       31706    31722      +16     
  Branches     5127     5154      +27     
==========================================
+ Hits        26014    26026      +12     
+ Misses       4937     4928       -9     
- Partials      755      768      +13     

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

[kylebuss]

Reviewed 6 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/attach-audio/attach-audio.component.spec.ts line 120 at r2 (raw file):

  }

  get firstIcon(): DebugElement {

nit Can this be changed to recordAudioButton()?

Code quote:

firstIcon()

[RaymondLuong3]

Nice work. In my testing, this looks to be working well. Make sure to add some acceptance tests too.

Reviewed 6 of 7 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @josephmyers and @kylebuss)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/attach-audio/attach-audio.component.spec.ts line 120 at r2 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

nit Can this be changed to recordAudioButton()?

I would add a class to the record audio button and then query for that class in addition to renaming the getter.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/checking_en.json line 44 at r2 (raw file):

    "tooltip_record": "Record audio",
    "tooltip_stop": "Stop recording",
    "tooltip_upload": "Upload audio"

I wonder if "Upload recording" might be a better name. When translating "audio" it might come across as "upload noise" but if we change this to "recording" that is a lot easier to get a clear translation. Of course I'm only speculating.

Code quote:

Upload audio"

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/attach-audio/attach-audio.component.scss line 20 at r2 (raw file):

}

.add-audio {

I'm not sure this background colour is necessary.

[josephmyers]

Thanks. Done!

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @kylebuss and @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/attach-audio/attach-audio.component.scss line 20 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I'm not sure this background colour is necessary.

Well, I'm not sure "necessary" is the right word here. Based on your screenshot, maybe you mean "visually pleasing in all situations"?

The color actually is necessary, unless we have another solution for the problem, because the problem it solves is that the cloud icon alone doesn't communicate that this button's function is specifically to upload audio files. If you look at the original Balsamiq mockups, my proposed design also included the word "Audio" within this area, to help users associate these two buttons together. Importantly, if users can see that these two buttons are related, they can probably figure out that the "cloud" is for audio, like the "mic" is. A cloud in isolation tells no one that it deals with audio.

So that's why it's necessary. Regarding it with the hover coloring, I tinkered with it for a while before settling on the current color, as the hover is visible against the background, and the background is clear enough to be seen/helpful.

If there's something about this that doesn't work, feel free to suggest an alternative or one of the other options in Balsamiq. Or maybe I've missed your comment altogether?

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/attach-audio/attach-audio.component.spec.ts line 120 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I would add a class to the record audio button and then query for that class in addition to renaming the getter.

"recordAudioButton" would be inaccurate, since it's not pulling anything specifically for recording. You can see this inaccuracy play out at line 67 of the spec file:

expect(env.firstIcon.nativeElement.textContent).toBe('clear');

The record button could never possibly say "clear." The method is pulling the first button icon it finds; in some cases it's for recording, in others it's for clearing/deleting.

I also don't like adding production code just for test purposes. But it's not a big deal in this small case. Done!

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/checking_en.json line 44 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

I wonder if "Upload recording" might be a better name. When translating "audio" it might come across as "upload noise" but if we change this to "recording" that is a lot easier to get a clear translation. Of course I'm only speculating.

We're using "audio" in plenty of other places in the software (notably the adjacent button), so my hope would be that the translators would reuse the same clear translation in this situation. Thoughts on this?

[kylebuss]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/attach-audio/attach-audio.component.scss line 20 at r2 (raw file):

Previously, josephmyers wrote…

Well, I'm not sure "necessary" is the right word here. Based on your screenshot, maybe you mean "visually pleasing in all situations"?

The color actually is necessary, unless we have another solution for the problem, because the problem it solves is that the cloud icon alone doesn't communicate that this button's function is specifically to upload audio files. If you look at the original Balsamiq mockups, my proposed design also included the word "Audio" within this area, to help users associate these two buttons together. Importantly, if users can see that these two buttons are related, they can probably figure out that the "cloud" is for audio, like the "mic" is. A cloud in isolation tells no one that it deals with audio.

So that's why it's necessary. Regarding it with the hover coloring, I tinkered with it for a while before settling on the current color, as the hover is visible against the background, and the background is clear enough to be seen/helpful.

If there's something about this that doesn't work, feel free to suggest an alternative or one of the other options in Balsamiq. Or maybe I've missed your comment altogether?

While I agree that a user might not associate both icons being audio related, I have to agree with Raymond that the background color may not be necessary.

1. Two icons indicate to the user that they have options.
2. The "mic" is associated with recording audio.
3. The "cloud" is associated with uploading a file of some type (in our case an audio file which is defaulted to upon clicking).
4. Hovering over the icon alerts the user to the function of the icon.
5. Consistency: In the Answers/Comments sections we use multiple icons without background color (granted they are not related).

[RaymondLuong3]

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/attach-audio/attach-audio.component.scss line 20 at r2 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

While I agree that a user might not associate both icons being audio related, I have to agree with Raymond that the background color may not be necessary.

1. Two icons indicate to the user that they have options.
2. The "mic" is associated with recording audio.
3. The "cloud" is associated with uploading a file of some type (in our case an audio file which is defaulted to upon clicking).
4. Hovering over the icon alerts the user to the function of the icon.
5. Consistency: In the Answers/Comments sections we use multiple icons without background color (granted they are not related).

Hmm, I see that reasoning now. The background colouring is not really a big issue, but I was pointing it out that on my first look, there didn't seem to be rationale for it. Perhaps we can leave it as is and update it as needed.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/checking_en.json line 44 at r2 (raw file):

Previously, josephmyers wrote…

We're using "audio" in plenty of other places in the software (notably the adjacent button), so my hope would be that the translators would reuse the same clear translation in this situation. Thoughts on this?

Yes, this looks reasonable. I agree there is already lots of places where we see audio as the string to localized. As long as we aren't getting negative feedback from people doing translations for us, it is probably not an issue :)

[kylebuss]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

[josephmyers]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/attach-audio/attach-audio.component.scss line 20 at r2 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Hmm, I see that reasoning now. The background colouring is not really a big issue, but I was pointing it out that on my first look, there didn't seem to be rationale for it. Perhaps we can leave it as is and update it as needed.

Thanks, sounds good. We certainly have other options, but I think this one is sufficient