SF-3029 Hide bottom bar while answering or commenting #2841
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2841 +/- ##
==========================================
+ Coverage 79.23% 79.24% +0.01%
==========================================
Files 534 534
Lines 31056 31060 +4
Branches 5063 5064 +1
==========================================
+ Hits 24606 24613 +7
- Misses 5659 5669 +10
+ Partials 791 778 -13 ☔ View full report in Codecov by Sentry. |
josephmyers
added
the
will require testing
There was a problem hiding this comment.
The code looks good, but with your PR I get an "NG0100: ExpressionChangedAfterItHasBeenCheckedError" error when I:
- Open a comment or answer for editing.
- Change the chapter number to a chapter without a question (it may also occur when changing chapter to another chapter with a question).
- The error dialog will appear.
See: https://youtu.be/-LVHAGvjveI
This occurs on mobile and desktop.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @josephmyers)
RaymondLuong3
force-pushed
the
feature/SF-3007-comment-audio
branch
from
eac859b to
58fca90
Compare
Nateowami
force-pushed
the
feature/SF-3007-comment-audio
branch
from
4893495 to
c8bdb59
Compare
josephmyers
force-pushed
the
improvement/SF-3029
branch
from
90e7aac to
8882492
Compare
There was a problem hiding this comment.
I had to do a bit of footwork to fix this, but I've fixed a couple bugs and oddities in the process. Please see my checkin comments for more details.
Reviewable status: 1 of 6 files reviewed, all discussions resolved (waiting on @pmachapman)
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:
pmachapman
added
ready to test
and removed
will require testing
josephmyers
force-pushed
the
improvement/SF-3029
branch
from
3112354 to
bd24933
Compare
|
@pmachapman I oopsied and forgot to read the JIRA description. After I did, I had to make two small but potentially impacting changes. Since you've already approved and |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
This helps the user focus on their immediate task and frees up vertical space. It is based off the presence of the CheckingInputComponent, which contains the textarea for keyboard entry. However, we don't want to base it off of having the textarea focused, since audio would cause it to reappear. It is better to rely solely on the presence of this component. When this property is set, it will in turn set a class which disables the display of the question-nav. This was necessary, as opposed to putting the textHasFocus check in the surrounding @if condition, to avoid "changed after checked" errors. I also experimented with animating the show/hide, but it didn't look good with all the other resizing (splitter + keyboard).
This fixes the NG0100 error when the bottom bar is hidden but the user then moves to a chapter with no questions. Hiding the bottom bar in this case also makes sense, given that the bottom bar is somewhat broken then. Since there is no selected question at the new chapter, the software maintains the selected question from the other chapter, meaning that the Next and Previous buttons don't make sense. The user can easily restore the bottom bar at this chapter by adding a question. This change is also required (mysteriously) for the followon change to fix the donut, which has been broken for quite some time.
This restores functionality to the donut, which is currently broken on Live. I'm not sure how this was ever working updating the view when the data changes, but my guess is that the callers were destroying and recreating the component whenever the data changed, avoiding this issue. I would imagine it's better practice to not require this.
This is per the JIRA task description
Per JIRA task description
Nateowami
force-pushed
the
improvement/SF-3029
branch
from
61999b6 to
68f3cc6
Compare
Nateowami
added
testing complete
This helps the user focus on their immediate task and frees up vertical space. It is based off the presence of the CheckingInputComponent, which contains the textarea for keyboard entry. However, we don't want to base it off of having the textarea focused, since audio would cause it to reappear. It is better to rely solely on the presence of this component.
When this property is set, it will in turn set a class which disables the display of the question-nav. This was necessary, as opposed to putting the textHasFocus check in the surrounding @if condition, to avoid "changed after checked" errors.
I also experimented with animating the show/hide, but it didn't look good with all the other resizing (splitter + keyboard).
This change is