Fix #5701: Alert user to save changes before publishing #5948

ankita240796 · 2018-12-06T08:19:54Z

Explanation

Fixes #5701
Added mechanism to alert user if user clicks on publish button without saving draft.

Here is a screenshot for the same:

Checklist

The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
The linter/Karma presubmit checks have passed.
- These should run automatically, but if not, you can manually trigger them locally using python scripts/pre_commit_linter.py and bash scripts/run_frontend_tests.sh.
The PR is made from a branch that's not called "develop".
The PR follows the style guide.
The PR is assigned to an appropriate reviewer.

ankita240796 · 2018-12-06T08:20:50Z

Hi @tjiang11, PTAL!

codecov-io · 2018-12-06T09:16:50Z

Codecov Report

Merging #5948 into develop will increase coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #5948      +/-   ##
===========================================
+ Coverage    45.47%   45.51%   +0.03%     
===========================================
  Files          518      518              
  Lines        30496    30505       +9     
  Branches      4576     4579       +3     
===========================================
+ Hits         13867    13883      +16     
+ Misses       16629    16622       -7

Impacted Files	Coverage Δ
...ev/head/pages/admin/roles_tab/RolesTabDirective.js	`1.61% <0%> (-0.06%)`	⬇️
...xploration_player/ExplorationPlayerStateService.js	`1.53% <0%> (-0.03%)`	⬇️
...head/pages/exploration_editor/ExplorationEditor.js	`5.22% <0%> (ø)`	⬆️
...plates/dev/head/services/StateRulesStatsService.js	`100% <0%> (ø)`	⬆️
...n_editor/statistics_tab/IssuesBackendApiService.js
..._editor/statistics_tab/PlaythroughIssuesService.js
...ead/pages/exploration_player/PlaythroughService.js
...ates/dev/head/services/PlaythroughIssuesService.js	`34.92% <0%> (ø)`
...ead/services/PlaythroughIssuesBackendApiService.js	`69.69% <0%> (ø)`
.../templates/dev/head/services/PlaythroughService.js	`86.77% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9adae1...bffbc7e. Read the comment docs.

DubeySandeep · 2018-12-06T17:46:24Z

Hi @ankita240796, Thanks for the PR!

@nithusha21, can you please review this PR as I'm not sure about the expected behaviour for the issue #5701.

nithusha21 · 2018-12-06T18:19:56Z

Hi @ankita240796 Thanks for the PR.

I am a little unsure myself about why this issue was filed. I think the issue was probably clicking a greyed out publish button, which was actually not clickable. In which case I think your fix is good.

However, I would suggest changing the error message to also include some context regarding open warnings that are not yet resolved. So if the creator saves the draft, and still there are open warnings, the publish button will still be disabled, and the pop-up still asks me to save draft (which I cannot anymore).

Probably, a better UX would be to have separate messages for either situation. So the current message is shown only when there are no open warnings and the draft isn't saved. Else, if there are warnings, we should show a message saying that. What do you think of this?

ankita240796 · 2018-12-06T18:23:51Z

Hi @nithusha21, I totally agree with your suggestion. It would be better to show the relevant error message to the user as per the situation. I will make the changes for the same.
Thanks!

ankita240796 · 2018-12-06T18:41:55Z

Hi @nithusha21, I have made the changes, PTAL!

seanlip · 2018-12-06T20:43:33Z

Hi all, I don't have enough context to comment usefully on the whole PR, but one thing I wanted to mention is that we never use in-built alert boxes -- they don't look great. Modals are the way to go here, as per other similar user interactions in Oppia.

Thanks!

nithusha21 · 2018-12-07T07:07:31Z

Hi Sean, thanks for those inputs! Yeah, I would agree with Sean. @ankita240796 could you make these alerts into Modals? You could search for uibModal in the codebase to get examples of how to create a modal. Let me know if you need help!

bansalnitish · 2018-12-07T07:31:06Z

Yeah, I too agree with Sean. We have never used in-built alerts. Let us know if you need any help. More than happy to help :)

ankita240796 · 2018-12-07T07:31:52Z

Hi @nithusha21 @seanlip @bansalnitish , Would it be better if instead of creating a modal we just try to display the tooltip when the button is clicked?

ankita240796 · 2018-12-07T09:56:47Z

Hi @nithusha21 @seanlip @bansalnitish

I have created a info modal to show the warning message to user if publish button is clicked without saving the drafts or resolving the warnings.

Here is a screenshot for the case when draft is not saved:

Do let me know if this looks fine or should we try displaying the tooltip on click.

Also, would it be better if we provide an option to save draft in the info modal itself?

Thanks!

nithusha21 · 2018-12-07T10:31:52Z

Hi @ankita240796 I think the modal looks great! Also I like the idea you have to add a button to save changes.

One suggestion I have is to drop the ? and change the title to "Cannot publish changes.".

Also, If we are doing this, do we want to continue to have the publish button greyed out in this situation? I am not entirely sure. Any suggestions @bansalnitish, @DubeySandeep, @aks681?

DubeySandeep · 2018-12-07T10:56:10Z

@nithusha21:

I think a grey button explains the button is disabled If it's disabled we should not allow them to be clickable (we can use this cursor instead of hand-cursor), in that case, we should use a tooltip.

If we are going to use modal then we should enable th Publish button.

Again, we use grey color "save draft" button when there is nothing to save, What are we going to do here for a consistent UI behaviour in both buttons?

Re "add a button to save changes.":
I liked the Idea, but what we are going to do if somone clicked the "Save draft" button in the modal, will we publish the exploration after saving draft? (In that case, I think sometimes we cannot publish exploration when there are warnings in the exploration (like doesn't have End exploration card).

ankita240796 · 2018-12-07T12:10:06Z

Hi @nithusha21 @DubeySandeep I tried out some of the stuff discussed in the comments above and have also pushed the code for the same.

Here is a gif for the same:

PTAL, I will change the code according to your inputs on this.

I also think that the publish button should not be greyed out if we are using a modal.

bansalnitish · 2018-12-07T19:20:07Z

I agree with @DubeySandeep that the save draft and publish should be consistent.

Also I think modals here look a bit unecessary, tooltip is a good way to display a simple message (the reason why its not working) like during publish we have the tooltip please resolve the warnings etc. We can add tooltip to both the save draft button and the publish button and display the tooltip accordingly like when to display resolve warnings, save draft, etc.

Any suggestions @DubeySandeep, @nithusha21, @ankita240796 ?

ankita240796 · 2018-12-08T04:14:05Z

I think I agree with @bansalnitish that a modal here would be too complex. It would be better to use a tooltip.
Also, if we are using modal for a Publish button, then we need to provide the same consistent behaviour for Save Draft button

nithusha21 · 2018-12-08T09:39:01Z

Hi @ankita240796, Sorry about the confusion on this PR! This is one of those issues for which we have no "correct" solution. There are many possibilities as outlined above. Could you write these down in a doc, and compare the pros and cons of these various choices? That way we will know what would be the best alternative to take.

Otherwise, I think every time you try something new, one of us will have a different solution, and we will not be able to finally resolve the issue! Does this sound good?

ankita240796 · 2018-12-08T10:06:09Z

Sure @nithusha21, I will do that!

ankita240796 · 2018-12-08T10:51:32Z

Hi @nithusha21 @DubeySandeep @bansalnitish

Here is the doc for the approaches we have discussed till now: https://docs.google.com/document/d/16ZdhCsBOlFtPpaw5cP2ZTcSOfgZmERJk_eDVROE2GlU/edit?usp=sharing

Please provide your inputs here.

Thanks!

bansalnitish · 2018-12-08T12:08:54Z

Nice work @ankita240796. I went through the doc and to me the tooltip still looks a better choice over the other (considering the pros are same and the corns for modal are high). Let's wait to listen from other two as well. Thanks!

bansalnitish · 2018-12-10T06:11:58Z

@nithusha21 could you give your inputs here?

nithusha21 · 2018-12-10T06:26:07Z

Hi @ankita240796 The doc looks great! Thanks!

I would lean towards the second solution, with a slight modification. Let a comment on the doc. PTAL!

ankita240796 · 2018-12-10T13:31:01Z

Hi @nithusha21 @DubeySandeep @bansalnitish

I have made the changes as discussed on the doc. Here is working demo for the same:

Also, I replaced the title element with tooltip since I was unable to find a way to display the title both on hover and click. This link also mentions that trying to show title on click as well as hover runs a risk of showing two tooltips at the same time.

PTAL, Thanks!

nithusha21

Hi @ankita240796 This looks much better. Thanks! The behavior LGTM. Just some stylistic comments, and queries. PTAL

nithusha21 · 2018-12-10T14:36:04Z

...plates/dev/head/pages/exploration_editor/exploration_save_and_publish_buttons_directive.html

-         title="<[getPublishExplorationButtonTooltip()]>">
+         uib-tooltip="<[getPublishExplorationButtonTooltip()]>"
+         tooltip-placement="bottom"
+         ng-class="{'div-disable': isExplorationLockedForEditing() || countWarnings()}" ng-click="showTooltip()">


is div-disable a standard class you want to use? If not, could you please find a more descriptive name. Maybe something like disable-publish-button or likewise.

I need to use this class both for publish and save draft buttons when they are disabled. Would it be better to use separate names for both?

I think separate names would be better.

nithusha21 · 2018-12-10T14:45:02Z

core/templates/dev/head/pages/exploration_editor/ExplorationSaveAndPublishButtonsDirective.js

@@ -117,6 +117,13 @@ oppia.directive('explorationSaveAndPublishButtons', [
                $scope.loadingDotsAreShown = false;
              });
          };
+
+          $scope.showTooltip = function() {


Could the function name also include some details about which Tooltip? showTooltip sounds a bit too generic.

nithusha21 · 2018-12-10T14:48:33Z

...plates/dev/head/pages/exploration_editor/exploration_save_and_publish_buttons_directive.html

@@ -69,3 +74,9 @@
    </div>
  </li>
 </ul>
+
+<style>
+  .div-disable {


This style will apply too generally. We should scope it to this specific directive.

Suggested change

.div-disable {

exploration-save-and-publish-buttons .div-disable {

nithusha21 · 2018-12-10T14:49:49Z

core/templates/dev/head/pages/exploration_editor/ExplorationSaveAndPublishButtonsDirective.js

@@ -117,6 +117,13 @@ oppia.directive('explorationSaveAndPublishButtons', [
                $scope.loadingDotsAreShown = false;
              });
          };
+
+          $scope.showTooltip = function() {
+            if ($scope.isExplorationLockedForEditing() ||


Break after the opening (

nithusha21 · 2018-12-10T14:52:49Z

...plates/dev/head/pages/exploration_editor/exploration_save_and_publish_buttons_directive.html

-         title="<[getPublishExplorationButtonTooltip()]>">
+         uib-tooltip="<[getPublishExplorationButtonTooltip()]>"
+         tooltip-placement="bottom"
+         ng-class="{'div-disable': isExplorationLockedForEditing() || countWarnings()}" ng-click="showTooltip()">


Just a query. If the button is disabled, will the ng-click event get triggered if clicked? If not when is the showTooltip function triggered? And why do we need it?

The ng-click on the div element gets triggered since there is no disable on the div.

Ah I see. Thanks for the explanation. But on hover itself the tooltip is shown right? how does the click help?

In the current working, when the user clicks the tooltip is gone but in current scenario, the tooltip stays on click. I am not sure if this is needed since the button is disabled which already implies user cannot click on it.

I think the tooltip should show immediately on hover as it does. I think disappearing on click is ok. (I tried on google docs, and their tooltips disappear on click). Also, when disabled, anyway the clicks would not register I think.

After removing, if on clicking the disabled button, the tooltip disappears, then lets keep this functionality. If on clicking the disabled button, the tooltip stays, let's get rid of the ng-clicks.

If that is the case, we can just use the title attribute and that will work fine. I will update the PR accordingly!

Done, PTAL!

nithusha21 · 2018-12-10T15:11:20Z

Also, please note some travis failures. I think what happens is the tooltip for "save draft" stays open on clicking the dropdown arrow, and prevents selecting "discard draft". We need to fix that :)

ankita240796 · 2018-12-10T15:30:25Z

I think what happens is the tooltip for "save draft" stays open on clicking the dropdown arrow, and prevents selecting "discard draft". We need to fix that :)

Hi @nithusha21, would hiding the tooltip after a small delay be a good solution here?

ankita240796 · 2018-12-10T15:40:21Z

I just figured out that changing the placement will fix the issue!

ankita240796 · 2018-12-10T16:14:29Z

Hi @nithusha21, I have made the changes, PTAL!

nithusha21 · 2018-12-10T16:36:43Z

...plates/dev/head/pages/exploration_editor/exploration_save_and_publish_buttons_directive.html

-         ng-click="showTooltip()">
+         tooltip-placement="right"
+         ng-class="{'disable-save-button': !isExplorationSaveable()}"
+         ng-click="showTooltipForDisabledButtons(disable-save-button)">


Shouldn't disable-save-button be a string?

nithusha21 · 2018-12-10T16:37:27Z

core/templates/dev/head/pages/exploration_editor/ExplorationSaveAndPublishButtonsDirective.js

-            if ($scope.isExplorationLockedForEditing() ||
+          $scope.showTooltipForDisabledButtons = function(buttonClass) {
+            if (
+              $scope.isExplorationLockedForEditing() ||


Can you indent this line and the next by 2 spaces? This will separate it from the following code.

It gives an indentation error: Expected indentation of 14 spaces but found 16 indent

Ah ok. I wasn't sure if it would do that. Fine to leave as-is.

nithusha21 · 2018-12-10T16:38:08Z

...plates/dev/head/pages/exploration_editor/exploration_save_and_publish_buttons_directive.html

@@ -3,7 +3,8 @@
    <div class="btn-group" style="margin-right: 10px; margin-top: 8px;"
         uib-tooltip="<[getPublishExplorationButtonTooltip()]>"
         tooltip-placement="bottom"
-         ng-class="{'div-disable': isExplorationLockedForEditing() || countWarnings()}" ng-click="showTooltip()">
+         ng-class="{'disable-publish-button': isExplorationLockedForEditing() || countWarnings()}"
+         ng-click="showTooltipForDisabledButtons(disable-publish-button)">


nithusha21 · 2018-12-10T16:44:23Z

Also, I think the placement down looks better. Every other option has the tooltip downwards. So can we try this, When the dropdown is open, can we hide the tooltip?

ankita240796 · 2018-12-10T19:26:03Z

Hi @nithusha21, I have made the changes as suggested as well as fixed the tooltip and dropdown conflict, PTAL!

nithusha21 · 2018-12-11T09:47:28Z

Hi Ankita, what was the reasoning behind removing the tooltip though? I thought the tooltip looked better. (also, all other buttons on the top have tooltips.). Lets do this:

Lets keep the tooltip on the publish changes button (which was the cause of the original issue)
The save draft can stay as a title (The button feels self explanatory, and I don't think the tooltip added too much info).

Does this sound good?

ankita240796 · 2018-12-11T10:35:06Z

Hi @nithusha21, I added the tooltip because it was easy to display that on click rather than a title attribute which was used earlier. If we just want the info on hover then a title attribute does the work, so I removed the tooltip and reverted to the original state of code.

Since the title also shows the same bubble as a tooltip and it is in bigger fonts, I feel that title looks better than a tooltip for these two buttons. What do you think?

Thanks!

nithusha21 · 2018-12-11T11:14:05Z

I didn't actually try out how the title tag works. Let me try it, and i'll let you know.

ankita240796 · 2018-12-11T13:08:07Z

Hi @nithusha21, here are some screenshots for clarification:

Using tooltip:

Using title:

nithusha21 · 2018-12-14T07:33:24Z

Yeah I agree visually they look the same. However I seem to prefer the tooltip as it shows up instantly on hover. The title takes a couple of seconds before appearing. So I would go with the tooltip, as it seems to address the original issue. Instantly on hover, the user is prompted with why it is not clickable. Do you agree?

nithusha21 · 2018-12-14T07:34:39Z

Also, regarding the click behaviour, I think it is not required. Only the hover behaviour of the tooltip looks to be useful to us. Once prompted that it is not clickable, it is very unlikely that someone will click it.

ankita240796 · 2018-12-14T13:37:09Z

Hi @nithusha21, I agree with your opinion on title and tooltip.

Lets keep the tooltip on the publish changes button (which was the cause of the original issue)
The save draft can stay as a title (The button feels self explanatory, and I don't think the tooltip added too much info).

I have made the changes as suggested by you, PTAL. Thanks!

nithusha21

I think it looks great now! Thanks for fixing this :)

LGTM @ankita240796

Alert user to save changes before publishing

ec30d34

DubeySandeep requested a review from nithusha21 December 6, 2018 17:41

DubeySandeep assigned nithusha21 Dec 6, 2018

Update warning message

d192b7d

Added info modal for publish button

9882286

Add option to save draft in info modal

5db6963

nithusha21 added the PR: don't merge - NEEDS CLA label Dec 9, 2018

nithusha21 removed the PR: don't merge - NEEDS CLA label Dec 10, 2018

ankita240796 added 2 commits December 10, 2018 18:38

Use tooltip and change cursor on disabled button

1950e83

Fix minor nit

78609b6

nithusha21 suggested changes Dec 10, 2018

View reviewed changes

Address review comments

a1125da

nithusha21 reviewed Dec 10, 2018

View reviewed changes

Fix tooltip and dropdown conflict

d465005

Remove tooltip on click, use title instead

59ac50f

ankita240796 force-pushed the fix-publish branch from 56daa4d to 59ac50f Compare December 10, 2018 19:48

Replace publish title with tooltip

bffbc7e

nithusha21 approved these changes Dec 14, 2018

View reviewed changes

nithusha21 merged commit f30bd81 into oppia:develop Dec 14, 2018

ankita240796 deleted the fix-publish branch December 14, 2018 17:34

	.div-disable {
	exploration-save-and-publish-buttons .div-disable {

Fix #5701: Alert user to save changes before publishing #5948

Fix #5701: Alert user to save changes before publishing #5948

Conversation

ankita240796 commented Dec 6, 2018 • edited by DubeySandeep Loading

Explanation

Checklist

ankita240796 commented Dec 6, 2018

codecov-io commented Dec 6, 2018 • edited Loading

Codecov Report

DubeySandeep commented Dec 6, 2018

nithusha21 commented Dec 6, 2018

ankita240796 commented Dec 6, 2018

ankita240796 commented Dec 6, 2018

seanlip commented Dec 6, 2018 • edited Loading

nithusha21 commented Dec 7, 2018

bansalnitish commented Dec 7, 2018

ankita240796 commented Dec 7, 2018 • edited Loading

ankita240796 commented Dec 7, 2018 • edited Loading

nithusha21 commented Dec 7, 2018

DubeySandeep commented Dec 7, 2018

ankita240796 commented Dec 7, 2018

bansalnitish commented Dec 7, 2018

ankita240796 commented Dec 8, 2018

nithusha21 commented Dec 8, 2018

ankita240796 commented Dec 8, 2018

ankita240796 commented Dec 8, 2018

bansalnitish commented Dec 8, 2018

bansalnitish commented Dec 10, 2018

nithusha21 commented Dec 10, 2018

ankita240796 commented Dec 10, 2018 • edited Loading

nithusha21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithusha21 commented Dec 10, 2018 • edited Loading

ankita240796 commented Dec 10, 2018

ankita240796 commented Dec 10, 2018 • edited Loading

ankita240796 commented Dec 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nithusha21 commented Dec 10, 2018

ankita240796 commented Dec 10, 2018

nithusha21 commented Dec 11, 2018

ankita240796 commented Dec 11, 2018

nithusha21 commented Dec 11, 2018

ankita240796 commented Dec 11, 2018

nithusha21 commented Dec 14, 2018

nithusha21 commented Dec 14, 2018

ankita240796 commented Dec 14, 2018

nithusha21 left a comment

Choose a reason for hiding this comment

ankita240796 commented Dec 6, 2018 •

edited by DubeySandeep

Loading

codecov-io commented Dec 6, 2018 •

edited

Loading

seanlip commented Dec 6, 2018 •

edited

Loading

ankita240796 commented Dec 7, 2018 •

edited

Loading

ankita240796 commented Dec 7, 2018 •

edited

Loading

ankita240796 commented Dec 10, 2018 •

edited

Loading

nithusha21 commented Dec 10, 2018 •

edited

Loading

ankita240796 commented Dec 10, 2018 •

edited

Loading