Fix #3093: Popover of Userhelp button remains constant when the user creates the exploration for the first time. #3267

shaz13 · 2017-04-01T17:01:37Z

#3093
The tooltip "To get help in future click here" doesn't disappear on the click and causes unintended behavior with profile menu.

How to produce error?

Login in locally with a new username or just use the test user name with admin panel.
Create an exploration. This should pop up the tooltip "To get help in future click here"
Now, click on the profile icon and try to go to any nav element.

Error

The menu simply disappears and user can't navigate to any page from menu

Expected behavior

The tooltip must be not cut off.
It should not block user from accessing menu.

This PR's fix

Added .popover class that overtakes thridparty.css class

codecov-io · 2017-04-01T17:57:05Z

Codecov Report

Merging #3267 into develop will increase coverage by 0.34%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #3267      +/-   ##
===========================================
+ Coverage     45.6%   45.95%   +0.34%     
===========================================
  Files          222      237      +15     
  Lines        18176    18350     +174     
  Branches      2931     2951      +20     
===========================================
+ Hits          8290     8433     +143     
- Misses        9886     9917      +31

Impacted Files	Coverage Δ
...es/exploration_editor/EditorNavigationDirective.js	`2.43% <0%> (-0.13%)`	⬇️
...re/templates/dev/head/pages/dashboard/Dashboard.js	`42.1% <0%> (-0.76%)`	⬇️
...emplates/dev/head/pages/preferences/Preferences.js	`41.41% <0%> (-0.7%)`	⬇️
core/templates/dev/head/filters.js	`62.03% <0%> (-0.12%)`	⬇️
...es/exploration_player/ConversationSkinDirective.js	`2.25% <0%> (-0.01%)`	⬇️
...nteractions/LogicProof/static/js/tools/teacher2.js	`92.85% <0%> (ø)`	⬆️
...ev/head/pages/exploration_editor/RouterServices.js	`0.92% <0%> (ø)`	⬆️
...ges/exploration_editor/settings_tab/SettingsTab.js	`0.6% <0%> (ø)`	⬆️
...sions/interactions/LogicProof/static/js/teacher.js	`88.63% <0%> (ø)`	⬆️
core/templates/dev/head/pages/profile/Profile.js	`1.31% <0%> (ø)`	⬆️
... and 52 more

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 cf97126...9f86a93. Read the comment docs.

jaredsilver

Thanks, @shaz13! This looks good to me. However, while it solves the problem of not being able to access the navigation dropdown, I'm going to loop in @anthkris for an opinion on whether this is sufficient or if we still want to also make the popover disappear after five seconds or on click.

@anthkris -- I feel like this is probably sufficient, but is there anything else I'm not considering?

anthkris · 2017-04-04T02:11:27Z

@jaredsilver @shaz13 My gut feeling is that it ought to fade out after several seconds (5 at least, I think). I mean it's just sitting there, tooltip in perpetuity. It's not in anyone's way anymore, but it might still be distracting.

Also @shaz13, I noticed that it doesn't travel with the page when the screen is resized. That seems like an issue.

shaz13 · 2017-04-04T17:02:01Z

@anthkris After going through the code-base realized that, "5 second" popup rule is already present.

After exploring and testing further. The current behavior seems apt with the code. The $scope value does changes from true to false. But, the catch is, it takes the whole page to reload to reflect the change in values. Hence, the popup never disappears within the 5 second timeout

Do correct me if my theory does not set right.

Please do suggest a further approach.

Shall I remove the existing script and replace with new one

or

Debug the present script, and load things using ajax or so?

Kindly do guide me @anthkris @jaredsilver
Thanks

anthkris · 2017-04-04T18:03:34Z

Okay @shaz13 I think I get what you're saying. So my recommendation would be to try and figure out how to get that script to work (in other words, debug first). If there's just no way it can work in it's current form, then try fix it some other way.

jaredsilver · 2017-04-05T01:43:10Z

I'm not terribly familiar with Angular, but from my very limited understanding what I suspect may be happening here is the view is not properly synchronizing with $scope. Again, I've never really worked with Angular, but one line of inquiry might be around $scope.$apply().

This might be of interest: https://nathanleclaire.com/blog/2014/01/31/banging-your-head-against-an-angularjs-issue-try-this/

The isLargeScreen interupted the postTutorialHelpPopoverIsShown= false value. Henced in the favour of expected behavior removed the ng-if condition.

shaz13 · 2017-04-05T12:12:12Z

@jaredsilver Thanks for the reference. After going through it realised that the present code was just fine but there was problems if ng-if condition of isLargeScreen as this doesn't change on timeout ng-if Boolean was true hence the popup didn't timeout. The code works fine as expected with removal of ng-if condition and Popup disappears after 5 seconds

@anthkris The popup inherits properties from thirdparty.css and making it dynamic with width is complex until thirdparty.css is modified or !important hack is used. And, travel with the page wasn't included before the PR too. I was wondering if it was intended behaviour as lines here in the codebase allow the popup only if Width >= 1024px

Would 5 second fade off rule will supplement for resizing?
Please, share your thoughts on it

Kindly, review the current commit.

Thanks

jaredsilver

Thanks a bunch, @shaz13!

I'm pretty happy with this on large screens, but removing the ng-if="ifLargeScreen" seems to mean that the popover will also appear on smaller screens:

(See upper right.)

It would be ideal if we could avoid this.

shaz13 · 2017-04-06T04:34:35Z

Sure. I have noticed it too. Thanks for pointing out. I will first experiment with another $scope for ng-if tag and make some changes to see if it works. Or will introduce a @media only class which should solve the large screen only issue.

…

On Apr 6, 2017 8:39 AM, "Jared Silver" ***@***.***> wrote: ***@***.**** requested changes on this pull request. Thanks a bunch, @shaz13 <https://github.com/shaz13>! I'm pretty happy with this on large screens, but removing the ng-if=ifLargeScreen seems to mean that the popover will also appear on smaller screens: [image: screen shot 2017-04-05 at 11 06 57 pm] <https://cloud.githubusercontent.com/assets/1312199/24736233/c9691cf4-1a54-11e7-86a2-e1ffb831a998.png> It would be ideal if we could avoid this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3267 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXToVeenMPQW2LrA1FPmk97ookSmNlxRks5rtFdVgaJpZM4MwjDY> .

isLargeScreen condition checked at the function $scope.$on('openPostTutorialHelpPopover', function()

shaz13 · 2017-04-06T15:59:54Z

@jaredsilver Thought about a CSS fix with media and display:none but as it would make things messy and unproductive adopted the pushed $scope function. Functionality remains same as before for isLargeScreen it is now operated at the directive code rather than inline. Kindly review it

jaredsilver

Thanks, @shaz13! LGTM from the design side of things!

jaredsilver · 2017-04-08T04:47:11Z

@kevinlee12 -- could you take a quick look to make sure the code here looks alright?

kevinlee12 · 2017-04-08T04:53:17Z

@jaredsilver, sure!

kevinlee12

Hi @shaz13, I have a minor comment. Once that is addressed, we can merge this.

kevinlee12 · 2017-04-08T05:15:54Z

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

@@ -38,10 +38,14 @@ oppia.directive('editorNavigation', [function() {
        $scope.postTutorialHelpPopoverIsShown = false;

        $scope.$on('openPostTutorialHelpPopover', function() {
-          $scope.postTutorialHelpPopoverIsShown = true;
-          $timeout(function() {
+          if ($scope.isLargeScreen) {


Move $scope.isLargeScreen above this function, this is so that we can get that defined before this gets a chance to trigger.

Actually -- I think everything gets initialized before anything runs, so the existing code is fine. Does it not trigger on your end?

Hmm, on second thought (and trial), it is fine, though it might be better if we shift those variable instantiation higher up for readability's sake.

@kevinlee12 openPostTutorialHelpPopover triggers when the user comes to create explorations for the first time.
If adopted this code:

if ($scope.isLargeScreen) { $scope.$on('openPostTutorialHelpPopover', function() { $scope.postTutorialHelpPopoverIsShown = true; $timeout(function() { $scope.postTutorialHelpPopoverIsShown = false; }, 5000); }); } else { $scope.postTutorialHelpPopoverIsShown = false; }

The helptool tip doesnt appear at all. Because, scripts of isLargeScreen execute first and misses openPostTutorialHelpPopover before it could initialize.

nope, that's not what I meant. I was speaking of moving line 103 above this function.

this line: $scope.isLargeScreen = (windowDimensionsService.getWidth() >= 1024);

@kevinlee12 Sure. Will push another commit on it

seanlip · 2017-04-08T05:21:45Z

core/templates/dev/head/pages/exploration_editor/editor_navigation_directive.html

@@ -81,3 +81,11 @@
    {% endif %}
  </ul>
 </script>
+<style type="text/css">
+  .popover {


This is too broad and needs to be scoped to just this directive.

@seanlip Sure. Could you please look into #1774 ? I am confused with what to push into the CSS after discovering this PR with similar work

That PR has the same mistake. /cc @shivanXI

@seanlip I was wondering about the merging too. If this PR goes first @shivanXI has to pull the changes before his merge occurs. If his goes first I should pull the changes. Either of one PR must re-write the whole code after merge and pull respective changes , Is it right?

Sorry, I don't know, and I can't look into that now, but you're right that if there are conflicts the person whose PR gets merged later must handle them. This is a normal part of shared repository dev workflows so I wouldn't worry too much about it -- but if your PRs overlap then you should coordinate and figure out what to do.

@seanlip Sure. Thanks

My 2 cents: just proceed with the PR as-is, only because #3277 is now focused on changing the color (which means that PR will have to remove parts where it duplicates work on this PR).

@kevinlee12 Sure.

kevinlee12

Hi @shaz13, just the CSS name, once you are done, please assign this PR back to me. thanks!

kevinlee12 · 2017-04-09T00:52:57Z

core/templates/dev/head/pages/exploration_editor/editor_navigation_directive.html

@@ -81,3 +81,11 @@
    {% endif %}
  </ul>
 </script>
+<style type="text/css">
+  ul .popover.bottom {


This is still problematic, can you introduce a new class name that is unique to the popover? What I am saying is to add another class along side dropdown

shaz13 · 2017-04-09T01:47:10Z

@kevinlee12, Just a confirmation. That this PR doesn't indent to solve the Popover issue instead it focuses on fading it off in 5 seconds. And as the other PR deals with CSS fixes, perhaps it would be better if I remove the over all CSS code from this PR and focus on just fade off in 5 second rule. What do you think

kevinlee12 · 2017-04-09T01:55:02Z

Since we've been changing the focus of the PR's quite a bit, I think we should keep it as is now to prevent further confusion and you've already implemented some of the features:

This PR:

Width corrections
Popover display duration

PR #3277:

Popover color change.

As Sean mentioned, the PR that gets merged later will have to deal with the name changes.

The div is introduce to bring the specificity because popover acts as class itself as well as dropdown is a class too. Hence binding it in div might avoid all future errors.

shaz13 · 2017-04-09T03:43:01Z

@kevinlee12 Done as suggested. Class is specific now.
The div is introduced in this commit to bring the specificity because popover acts as class (.popover) itself as well as dropdown is a class too. Hence binding it in div might avoid all future errors as these both will be nested under .oppia-help-dropdown class.

Please review the code (assigning it to you)

kevinlee12

Just a nit.

kevinlee12 · 2017-04-09T06:09:27Z

core/templates/dev/head/pages/exploration_editor/editor_navigation_directive.html

@@ -70,14 +70,24 @@
    </li>

    {% if username %}
-      <li ng-if="isLargeScreen" class="dropdown" popover-is-open="postTutorialHelpPopoverIsShown"
+    <div class="oppia-help-dropdown nav navbar-nav oppia-navbar-nav pull-right ng-cloak">


The additional class should be in the line below, not this one. (We're manipulating the popover directly, not this class)

@kevinlee12 I have tried making changes so, but the alignment and the navbar-nav settings breaks or popover are not functional when adding it to dropdown.
Could you please confirm the changes locally? Or if I misinterpreted could you quote the code snippet for clarity.

Interestingly, popover is acting alike an element here. Modifying it anything else, breaks one or other thing. (nav bar, material icon or the popover itself)

It gives error if adopt so;
<li class="oppia-help-dropdown dropdown" popover-is-open="postTutorialHelpPopoverIsShown" popover-placement="bottom" popover-trigger="none" popover="To get help in the future, click here!">
As popover is inline with the <li>tag itself

(as mentioned in my other comment), we can keep this placement. I was wondering if you can experiment with oppia-navbar-breadcrumb instead of the CSS you've defined (so that we can get a consistent experience for our users). No need to push, but do report back to this thread. thanks!

However, you do want to move oppia-help-dropdown to the right of oppia-navbar-nav

<div class="oppia-help-dropdown nav navbar-nav oppia-navbar-nav pull-right ng-cloak">
to
<div class="nav navbar-nav oppia-navbar-nav oppia-help-dropdown pull-right ng-cloak">
@kevinlee12 This sounds good?

Thanks! Will push another commit now. Suggesting this change.

kevinlee12 · 2017-04-09T06:27:05Z

You are doing it so that its like `class="dropdown oppia-help-dropdown"` right?

…

On Sat, Apr 8, 2017 at 11:23 PM, Shahebaz ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In core/templates/dev/head/pages/exploration_editor/editor_ navigation_directive.html <#3267 (comment)>: > @@ -70,14 +70,24 @@ </li> {% if username %} - <li ng-if="isLargeScreen" class="dropdown" popover-is-open="postTutorialHelpPopoverIsShown" + <div class="oppia-help-dropdown nav navbar-nav oppia-navbar-nav pull-right ng-cloak"> @kevinlee12 <https://github.com/kevinlee12> I have tried making changes so, but the alignment and the navbar-nav settings breaks or popover are not functional when adding it to dropdown. Could you please confirm locally? Or if I misinterpreted could you quote the code snippet for clarity. Interestingly, popover is acting alike an element here. Modifying it anything else, breaks one or other thing. (nav bar, material icon or the popover itself) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3267 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFfdMJUZppJX9-kvoF_4bWo6xW9P2yRlks5ruHlagaJpZM4MwjDY> .

shaz13 · 2017-04-09T06:29:09Z

@kevinlee12 Yes, exactly. It doesnt work.

<li class="dropdown oppia-help-dropdown" popover-is-open="postTutorialHelpPopoverIsShown"
          popover-placement="bottom" popover-trigger="none"
          popover="To get help in the future, click here!">

kevinlee12 · 2017-04-09T07:02:20Z

Hmm, after looking how they implemented the CSS regarding the other popovers, your approach earlier is correct.

shaz13 · 2017-04-09T07:18:56Z

@kevinlee12 Thanks!. Earlier refers to latest commit or the previous ul .popover class one?

kevinlee12

Nit

kevinlee12 · 2017-04-09T07:19:50Z

core/templates/dev/head/pages/exploration_editor/editor_navigation_directive.html

@@ -70,14 +70,24 @@
    </li>

    {% if username %}
-      <li ng-if="isLargeScreen" class="dropdown" popover-is-open="postTutorialHelpPopoverIsShown"
+    <div class="oppia-help-dropdown nav navbar-nav oppia-navbar-nav pull-right ng-cloak">


However, you do want to move oppia-help-dropdown to the right of oppia-navbar-nav

kevinlee12 · 2017-04-09T07:21:45Z

@shaz13, latest commit that you have on Github (f8a4d88)

shaz13 · 2017-04-09T07:30:54Z

@kevinlee12 Done. Pushed new commit. Kindly review it

kevinlee12

lgtm!

Fixed Popover of Userhelp button

4b34d04

Added .popover class that overtakes thridparty.css class

shaz13 self-assigned this Apr 1, 2017

shaz13 requested a review from jaredsilver April 1, 2017 17:07

jaredsilver reviewed Apr 3, 2017

View reviewed changes

anthkris mentioned this pull request Apr 4, 2017

Hard to find tutorial reminder bubble in editor #1774

Closed

5 second popup rule active

97be15f

The isLargeScreen interupted the postTutorialHelpPopoverIsShown= false value. Henced in the favour of expected behavior removed the ng-if condition.

jaredsilver suggested changes Apr 6, 2017

View reviewed changes

shaz13 added 2 commits April 6, 2017 21:23

isLargeScreen condition checked at the function

e89c81a

isLargeScreen condition checked at the function $scope.$on('openPostTutorialHelpPopover', function()

Indent fixes

e803ca1

Linter fix

95c67df

jaredsilver approved these changes Apr 8, 2017

View reviewed changes

kevinlee12 self-requested a review April 8, 2017 04:53

kevinlee12 suggested changes Apr 8, 2017

View reviewed changes

seanlip reviewed Apr 8, 2017

View reviewed changes

kevinlee12 mentioned this pull request Apr 8, 2017

Fix #1774: Changed color combination required for visibility of reminder bubble in editor. #3277

Merged

Scoped CSS and isLargeScreen scope replacement

f475eb8

kevinlee12 suggested changes Apr 9, 2017

View reviewed changes

Class specific with div

f8a4d88

The div is introduce to bring the specificity because popover acts as class itself as well as dropdown is a class too. Hence binding it in div might avoid all future errors.

shaz13 assigned kevinlee12 Apr 9, 2017

kevinlee12 unassigned shaz13 Apr 9, 2017

kevinlee12 suggested changes Apr 9, 2017

View reviewed changes

kevinlee12 assigned shaz13 and unassigned kevinlee12 Apr 9, 2017

kevinlee12 suggested changes Apr 9, 2017

View reviewed changes

oppia-help-dropdown classe sorted

9f86a93

kevinlee12 approved these changes Apr 9, 2017

View reviewed changes

kevinlee12 added the PR: LGTM label Apr 9, 2017

kevinlee12 merged commit 3914ba1 into oppia:develop Apr 9, 2017

shaz13 deleted the help-tooltip-fix branch April 12, 2017 14:51

This was referenced Aug 13, 2017

Fix part #3400 Objective field directive #3740

Merged

Exploration creator "User Help" tooltip not disappearing as it's supposed to #3764

Closed

Fix #3093: Popover of Userhelp button remains constant when the user creates the exploration for the first time. #3267

Fix #3093: Popover of Userhelp button remains constant when the user creates the exploration for the first time. #3267

Conversation

shaz13 commented Apr 1, 2017 • edited Loading

codecov-io commented Apr 1, 2017 • edited Loading

Codecov Report

jaredsilver left a comment

Choose a reason for hiding this comment

anthkris commented Apr 4, 2017

shaz13 commented Apr 4, 2017 • edited Loading

anthkris commented Apr 4, 2017

jaredsilver commented Apr 5, 2017

shaz13 commented Apr 5, 2017 • edited Loading

jaredsilver left a comment • edited Loading

Choose a reason for hiding this comment

shaz13 commented Apr 6, 2017 via email

shaz13 commented Apr 6, 2017 • edited Loading

jaredsilver left a comment

Choose a reason for hiding this comment

jaredsilver commented Apr 8, 2017

kevinlee12 commented Apr 8, 2017

kevinlee12 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

shaz13 Apr 8, 2017 • edited Loading

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

kevinlee12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaz13 commented Apr 9, 2017 via email • edited Loading

kevinlee12 commented Apr 9, 2017

shaz13 commented Apr 9, 2017 • edited Loading

kevinlee12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaz13 Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

shaz13 Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

kevinlee12 Apr 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shaz13 Apr 9, 2017 • edited Loading

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

kevinlee12 commented Apr 9, 2017 via email

shaz13 commented Apr 9, 2017

kevinlee12 commented Apr 9, 2017

shaz13 commented Apr 9, 2017

kevinlee12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinlee12 commented Apr 9, 2017

shaz13 commented Apr 9, 2017

kevinlee12 left a comment

Choose a reason for hiding this comment

shaz13 commented Apr 1, 2017 •

edited

Loading

codecov-io commented Apr 1, 2017 •

edited

Loading

shaz13 commented Apr 4, 2017 •

edited

Loading

shaz13 commented Apr 5, 2017 •

edited

Loading

jaredsilver left a comment •

edited

Loading

shaz13 commented Apr 6, 2017 •

edited

Loading

shaz13 Apr 8, 2017 •

edited

Loading

shaz13 commented Apr 9, 2017 via email •

edited

Loading

shaz13 commented Apr 9, 2017 •

edited

Loading

shaz13 Apr 9, 2017 •

edited

Loading

shaz13 Apr 9, 2017 •

edited

Loading

kevinlee12 Apr 9, 2017 •

edited

Loading

shaz13 Apr 9, 2017 •

edited

Loading