SF-3114 Make chapter audio only hide bottom bar if scripture text is shown

[josephmyers]

This fixes an issue where "hide text" projects couldn't navigate questions on mobile, since the bottom bar is now being hidden.

So now, we don't hide the bottom bar if "hide text" is enabled. We will instead only hide the bar if the "hide text" is disabled and the chapter audio is shown. Effectively, this is when "the user opens the chapter audio."

The second case where we hide the bar remains. I did a slight refactoring to make all this clear and readable.

The original reason for the hiding behavior is still intact, that being to free up space on mobile and keep users focused on their intended task.

---

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.84%. Comparing base (e3e072a) to head (addcd7e).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2905   +/-   ##
=======================================
  Coverage   80.84%   80.84%           
=======================================
  Files         532      532           
  Lines       31119    31121    +2     
  Branches     5064     5061    -3     
=======================================
+ Hits        25158    25161    +3     
+ Misses       5210     5198   -12     
- Partials      751      762   +11     

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

[Nateowami]

@josephmyers Please update the PR title to conform to the guidelines in CONTRIBUTING.md.

I know the meaning of the title might seem obvious to you, but very often there are two completely contradictory interpretations that can't be differentiated without knowing what the original problem was.

Chapter audio only hides bottom bar if scripture text is shown

Upon reading this title, my first thought was that the bottom bar wasn't being hidden in a situation where it should, but it sounds like it's the exact opposite. Sometimes people create PRs that describe the problem (e.g. #1539), but you can't tell whether a title is describing the problem or the solution without context. On the other hand, if you always use imperative mood, there isn't nearly as much room for ambiguity, and it's easier to read a list of commit messages that have the same grammatical structure.

[RaymondLuong3]

I agree, the commit message would be better if it describe what was done to the repo. Something like "Fix bottom nav hidden if hide scripture text enabled" describes the problem that was resolved without being ambiguous.

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

---

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

  }

  get textboxIsShown(): boolean {

Nit: This could be private.

Code quote:

  get textboxIsShown(): boolean {

[josephmyers]

To be fair, the commit message does describe what was done to the repo. Nathaniel's complaint is that you could interpret that message differently, I guess as if my checkin were saying that is the problem.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @josephmyers)

[josephmyers]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: This could be private.

You could, but I don't see a need to. The only thing I could think of is maybe API clutter, but we're way beyond that for this class.

Readonly properties like this are almost always valuable (or neutral) to expose. It's the modifiable fields, e.g. simple boolean flags, that can be harmful when exposed. It's impossible for this property to cause a bug (unless of course there's a bug in the logic here that I don't see).

[RaymondLuong3]

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

---

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

Previously, josephmyers wrote…

You could, but I don't see a need to. The only thing I could think of is maybe API clutter, but we're way beyond that for this class.

Readonly properties like this are almost always valuable (or neutral) to expose. It's the modifiable fields, e.g. simple boolean flags, that can be harmful when exposed. It's impossible for this property to cause a bug (unless of course there's a bug in the logic here that I don't see).

That is a far point. Would it not be possible to assign the getter to be something else like () => true? and then the getter would not work? I assume not but typescript lets you do unexpected things sometimes =P

[josephmyers]

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

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

That is a far point. Would it not be possible to assign the getter to be something else like () => true? and then the getter would not work? I assume not but typescript lets you do unexpected things sometimes =P

Yes, that would be really weird. Maybe too weird even for TS!