Fix #3644: Handles empty hint in hint editor by showing a message and deleting it #3648
Conversation
|
Did you test this carefully? I poked around and ran into cases where it does not fully work -- e.g. if you clear hint 1 and then click on hint 2. |
| var currentActiveIndex = $scope.activeHintIndex; | ||
| if (currentActiveIndex && ( | ||
| !stateHintsService.displayed[currentActiveIndex].hintText)) { | ||
| alertsService.addInfoMessage('Deleting empty hint!'); |
There was a problem hiding this comment.
I think I'd drop the exclamation mark here, and follow the style of the other info messages -- deleting a hint is not that exciting :P
There was a problem hiding this comment.
haha, sure! (and yes, that particular case doesn't work. I'll fix it.)
Codecov Report
@@ Coverage Diff @@
## develop #3648 +/- ##
===========================================
- Coverage 44.85% 44.83% -0.02%
===========================================
Files 257 257
Lines 19881 19888 +7
Branches 3133 3135 +2
===========================================
Hits 8917 8917
- Misses 10964 10971 +7
Continue to review full report at Codecov.
|
| @@ -51,6 +51,13 @@ oppia.controller('StateHints', [ | |||
| }; | |||
|
|
|||
| $scope.changeActiveHintIndex = function(newIndex) { | |||
| var currentActiveIndex = $scope.activeHintIndex; | |||
| if (Number.isInteger(currentActiveIndex) && ( | |||
There was a problem hiding this comment.
Semantically, should this actually be something like currentActiveIndex !== null?
| @@ -51,6 +51,13 @@ oppia.controller('StateHints', [ | |||
| }; | |||
|
|
|||
| $scope.changeActiveHintIndex = function(newIndex) { | |||
| var currentActiveIndex = $scope.activeHintIndex; | |||
| if (Number.isInteger(currentActiveIndex) && ( | |||
| !stateHintsService.displayed[currentActiveIndex].hintText)) { | |||
There was a problem hiding this comment.
You can indent this by 2 more to distinguish it from the next line (which is the one that actually starts an inner scope).
| alertsService.addInfoMessage('Deleting empty hint.'); | ||
| stateHintsService.displayed.splice(currentActiveIndex, 1); | ||
| stateHintsService.saveDisplayedValue(); | ||
| } | ||
| // If the current hint is being clicked on again, close it. |
There was a problem hiding this comment.
sorry, small nit: indent this by 2.
There was a problem hiding this comment.
I think I'll fix the "Hints" showing up in logged out view in this PR along with this.
There was a problem hiding this comment.
OK, sounds good. Let me drop the lgtm for the time being while you do that, then :)
|
Fixed "Hints" text appearing when there are no hints in logged out view as well. PTAL. Thanks! |
Linking #3644.