ember-popovers #92

a15n · 2016-08-29T20:00:41Z

ember-popovers

This PR addresses the need for a popover expressed here (#77) and implied here (#51)

creates popover-on-element and popover-on-component components which extend their tooltip counterparts
- these all have the same API options as ember tooltips
- these behave the exact same way EXCEPT
  - event handling is custom and mirrors that found in Twitter popovers.
  - a new hideDelay param which defines the milliseconds a user may hover outside of the popover before it hides. This is necessary because popovers can be spaced N pixels away from their target element
  - popovers have no visual CSS properties. These must be defined by the user
adds popover specific tests in the tests/integration/components/popover/ directory
exposes a hide action for use within the popover's block (ie clicking an interior close button)

Exhaustive list of changes made to tooltip-on-element

git mv tooltip-on-element.js tooltip-and-popover.js (c5da8b7)
moved the tooltip-specific event handling into tooltip-on-element's didInsertElement (2b99f85)
use classPrefix instead of one-off strings. Change classPrefix to tooltip-and-popover (9dc2fc7)
used classPrefix: 'ember-tooltip-or-popover', (1e9cef2 and 5045c4c)
deprecated tooltipIsVisible in favor of isShown (110c0fa)
deprecated onTooltipShow in favor of onShow, etc (5993997)
replaced tooltipCounterId with tooltipOrPopoverCounterId (26fcdd4)

Popover event="click"

clicking the target element will toggle the popover
clicking inside the popover will not toggle it
clicking outside of the target and the popover will hide it

Popover event="hover" (not suitable for mobile)

hovering on the target element will show the popover
hovering away from the target element will hide the popover after hideDelay milliseconds (default 250)
hovering from the target element to the popover works if it happens during the hideDelay

Popover focus (available when event="focus", "click", or "hover"

focusing into the target will show the popover
focusing into the popover will keep the popover visible
focusing out of the popover and target will hide the popover

Popover hide API

invoke the popover like so and gain access to the hide action

{{#popover-on-element as |popover|}}
    Click <a href {{action popover.hide}}>here</a> to hide popover
{{/popover-on-element}}

TODO (Blocking)

a15n · 2016-08-29T20:17:48Z

even with the "initial commit" commit the tests are failing...

https://travis-ci.org/sir-dunxalot/ember-tooltips/jobs/156037676

MiguelMadero · 2016-08-29T23:12:25Z

addon/templates/components/popover-on-element.hbs

@@ -0,0 +1,3 @@
+{{yield (hash
+	hide=(action "hide")


minor: indent

this is a github display issue. It is correctly indented in my IDE

a15n · 2016-08-29T23:26:50Z

package.json

-    "ember-cli-sass": "5.3.1"
+    "ember-cli-htmlbars": "^1.1.0",
+    "ember-cli-sass": "5.3.1",
+    "ember-hash-helper-polyfill": "^0.1.1"


we need ember-hash-helper-polyfill and ember-cli-htmlbars because of the template that passes the hide closure action to the yield via the hash helper

ember-hash-helper-polyfill is only needed till Ember 2.3

sir-dunxalot · 2016-08-30T15:46:57Z

Hey @a15n - any idea why the tests are failing on the initial commit? Could it be as simple as merging with master?

a15n · 2016-08-30T16:34:21Z

@sir-dunxalot I made sure to use the most up to date version before I started. I'm checking my git log and I have the 2.0.1 release and all the subsequent commits. When I run the tests locally I get the following errors. These are the same errors I noticed when I tested the 'initial commit'.

Died on test #2     at testWrapper (http://localhost:4200/assets/test-support.js:6996:11)
at test (http://localhost:4200/assets/test-support.js:7010:39)
at Module.callback (http://localhost:4200/assets/tests.js:2217:24)
at Module.exports (http://localhost:4200/assets/vendor.js:132:32)
at requireModule (http://localhost:4200/assets/vendor.js:32:18)
at s (http://localhost:4200/assets/vendor.js:67058:143)
at s (http://localhost:4200/assets/vendor.js:67058:123): Cannot read property 'classList' of null

a15n · 2016-08-30T16:36:42Z

Also...

sir-dunxalot · 2016-08-30T18:28:34Z

Hmm, ok. I think either you or I will have to work on this branch and get it passing before merging in. With a PR this big I'd rather not merge onto master and then get things to pass (incase others are pulling down master into their projects directly). I could pull down from your branch if preferred.

Alternatively, we could reissue this PR to a popover branch on this repo and I could try and get the tests to pass on that before merging it into master.

Let me know how you would like to proceed. I know you've done tons of work on this so I don't want to have you to put in more work especially if it's unrelated to this feature.

a15n · 2016-08-30T18:43:44Z

"With a PR this big I'd rather not merge onto master and then get things to pass (incase others are pulling down master into their projects directly)"

Agreed. If you were able to pull down this branch and investigate that would be great. There are popover tests failing right now but I am in the process of fixing those.

Thanks

a15n · 2016-08-30T19:01:12Z

@sir-dunxalot regarding class name structure I have .ember-tooltip (no changes), .ember-popover, and .tooltip-and-popover for the CSS attributes that are shared by both. What do you think would be best?

MiguelMadero · 2016-08-31T16:59:01Z

addon/components/popover-on-element.js

@@ -40,25 +40,28 @@ export default TooltipAndPopoverComponent.extend({

    } else if (_showOn === 'click') {

-      $target.on(_showOn, (event) => {
+      $(document).on('click', (event) => {


Lets make sure you unsubscribe from all events, but specially this one.

a15n · 2016-08-31T17:33:36Z

addon/components/popover-on-element.js

+      return;
+    }
+
+    if (_showOn === 'mouseenter') {


I think I'll be able to simplify the _showOn == 'mouseenter' case now that I only use _isMouseInside for that approach.

a15n · 2016-09-06T15:36:12Z

Yes the code portion is all set now. Things have gotten rather busy for me at work so I don't think I'll be able to help with updating the resources.

We are good to wait till this weekend. Thanks!

sir-dunxalot · 2016-09-06T21:42:52Z

Ok, great. I'll wait to merge this in until I have the documentation in place. I don't mind a 10-minute overlap of merging without the docs updated so I'll do it on the fly. I'll also do a QA run through - will keep you updated.

jamesdixon · 2016-09-06T22:26:31Z

Well done, gents! Looking forward to trying this out.

andrew-zenefits · 2016-09-07T22:27:52Z

@sir-dunxalot you'll notice in this latest PR that I have improved the jQuery checks to determine if an element is inside the popover or elsewhere. I tested out the new popovers in our code base and learned that sometimes all of the tethers are contained within a div at the bottom, rather than within the target. For this reason the following code didn't work and this fix was necessary..

$target.find(clickedElement).length;
// didn't work because sometimes the popover was outside of the target

3248b18

jliuzen · 2016-09-07T22:40:23Z

addon/components/popover-on-element.js

+  hideDelay: '250',
+
+  layout,
+  classNames: ['ember-popover'],


nit/personal preference: put classNames at top (above hideDelay)

andrew-zenefits · 2016-09-08T03:50:55Z

@sir-dunxalot, @MiguelMadero you'll notice in the last commit I've triggered every event='focus' test event with javascript instead of jQuery. While investigating the flaky event='focus' tests we discovered that each test would fail the second time it was called, unless the chrome screen was clicked just before the test was run. We suspected that the trigger('focus') events weren't being triggered correctly the second time. Miguel mentioned something about synthetic events being introduced in Ember 2.8. We think that Travis is being affected by the same thing.

ff262b6

jamesdixon · 2016-09-16T04:32:12Z

How's this going guys? Anxious to give it a shot!

andrew-zenefits · 2016-09-24T00:44:02Z

@sir-dunxalot is there anything I can do to help this land?

sir-dunxalot · 2016-09-24T13:06:19Z

Hey Andrew, it's honestly just documentation in the README. I've worked 14 days straight and still haven't gotten to it. My apologies. Let me know if you're able to push this over the line.

a15n · 2016-09-29T02:36:29Z

@sir-dunxalot I'm glad I was able to work on this some more. I added backwards compatibility for tooltipIsVisible and the onTooltipShow, etc parameters. I have added tests to support these properties. I recommend we plan on removing support for these properties in v3.0.0.

I have also updated the readme to include popovers.

I believe this is ready to merge, BUT it still has flaky tests in ember-beta. I'm at a loss for how to address the flaky tests because I cannot get them to fail locally.

a15n · 2016-10-03T21:54:58Z

@sir-dunxalot I've addressed some of the failing tests here. #97

Let me know if I can help in any other way to land this.

sir-dunxalot · 2016-10-05T01:56:40Z

Andrew, I'm finally getting around to this. This has been an immense effort - great work, truly.

#97 is now on master. It looks like that has caused merge conflicts. If we can fix those so that there's only the one test failure (the el.classList is not an object error) then I'm happy to merge this, fix that one test locally, and release.

a15n · 2016-10-05T17:51:19Z

@sir-dunxalot I have merged with the newest master, resolved conflicts, and all tests are passing except the Integration | Component | tooltip on component tests on ember-beta and ember-canary. I think we're good to go?

sir-dunxalot · 2016-10-05T23:24:48Z

Alright, I'm merging and pulling down now. I will try and get those last two test passing before releasing.

sir-dunxalot · 2016-10-06T00:28:48Z

I've got four tests failing locally with the following error:

'[object EventConstructor]' is not a constructor (evaluating 'new window.Event('focus')')

I'm attempting to fix these before fixing the remaining two tooltip on component failing tests.

a15n added 3 commits August 29, 2016 12:59

initial commit

35c3f5f

git mv tooltip-on-element -> tooltip-and-popover

c5da8b7

add back original tooltip-on-element.js

3247408

a15n added 8 commits August 29, 2016 13:20

move .on event handling into tooltip-on-element.didInsertElement

2b99f85

add popover tests for TDD

e583c74

split the classes. removed transition to test visibility (will revert)

3758f3b

handle the popover events correctly

451d6f8

fixed the CSS

53c71be

make popover-on-component and add popover-specific tests

69a0c8c

addressed feedback: no private API, shared -on-component util

1b27d6a

first attempt at using (hash

fa87fd4

MiguelMadero reviewed Aug 29, 2016
View reviewed changes

second attempt: hash working, uses layout

eec6611

a15n reviewed Aug 29, 2016
View reviewed changes

clean up and fixing bugs

1b22b7c

a15n added 4 commits August 30, 2016 13:54

use classPrefix instead of one-off strings

9dc2fc7

better popover-on-element.didInsertElement code

4fc2c8e

use window.click to handle click outside events

cfc9ba8

update tests

fedf79c

MiguelMadero reviewed Aug 31, 2016
View reviewed changes

unsubscribe from all events on willDestroyElement

4afd37e

a15n reviewed Aug 31, 2016
View reviewed changes

andrew-zenefits mentioned this pull request Sep 6, 2016

make ember-popovers OLDZenefits/ember-tooltips#8

Merged

use .in & check popover.in and popover.find

3248b18

jliuzen reviewed Sep 7, 2016
View reviewed changes

make hideDelay an integer

4c45106

andrew-zenefits mentioned this pull request Sep 8, 2016

ember popover OLDZenefits/ember-tooltips#16

Merged

focus events triggered without jQuery

ff262b6

a15n added 4 commits September 28, 2016 17:58

remove className which didn't apply CSS styles

5045c4c

add backwards compatibility for tooltipIsVisible

110c0fa

add backwards support for onTooltip____ actions

5993997

update the README

b5c05a9

a15n added 2 commits October 5, 2016 09:42

merged master into ember-popover. addressed conflicts

02d2393

increase timeout for popover focus tests

70608ed

sir-dunxalot merged commit 0171fbf into sir-dunxalot:master Oct 5, 2016

sir-dunxalot mentioned this pull request Oct 14, 2016

Popover Component Feature Request #77

Closed

sir-dunxalot mentioned this pull request Dec 22, 2016

Adding a link inside the tooltip #51

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ember-popovers #92

ember-popovers #92

a15n commented Aug 29, 2016 •

edited

Loading

a15n commented Aug 29, 2016 •

edited

Loading

MiguelMadero Aug 29, 2016

a15n Aug 29, 2016

a15n Aug 29, 2016 •

edited

Loading

a15n Sep 1, 2016

sir-dunxalot commented Aug 30, 2016

a15n commented Aug 30, 2016 •

edited

Loading

a15n commented Aug 30, 2016

sir-dunxalot commented Aug 30, 2016

a15n commented Aug 30, 2016

a15n commented Aug 30, 2016

MiguelMadero Aug 31, 2016

a15n Aug 31, 2016

a15n commented Sep 6, 2016

sir-dunxalot commented Sep 6, 2016

jamesdixon commented Sep 6, 2016

andrew-zenefits commented Sep 7, 2016

jliuzen Sep 7, 2016 •

edited

Loading

andrew-zenefits commented Sep 8, 2016 •

edited

Loading

jamesdixon commented Sep 16, 2016

andrew-zenefits commented Sep 24, 2016

sir-dunxalot commented Sep 24, 2016

a15n commented Sep 29, 2016

a15n commented Oct 3, 2016

sir-dunxalot commented Oct 5, 2016 •

edited

Loading

a15n commented Oct 5, 2016

sir-dunxalot commented Oct 5, 2016

sir-dunxalot commented Oct 6, 2016

ember-popovers #92

ember-popovers #92

Conversation

a15n commented Aug 29, 2016 • edited Loading

ember-popovers

Exhaustive list of changes made to tooltip-on-element

Popover event="click"

Popover event="hover" (not suitable for mobile)

Popover focus (available when event="focus", "click", or "hover"

Popover hide API

TODO (Blocking)

a15n commented Aug 29, 2016 • edited Loading

MiguelMadero Aug 29, 2016

Choose a reason for hiding this comment

a15n Aug 29, 2016

Choose a reason for hiding this comment

a15n Aug 29, 2016 • edited Loading

Choose a reason for hiding this comment

a15n Sep 1, 2016

Choose a reason for hiding this comment

sir-dunxalot commented Aug 30, 2016

a15n commented Aug 30, 2016 • edited Loading

a15n commented Aug 30, 2016

sir-dunxalot commented Aug 30, 2016

a15n commented Aug 30, 2016

a15n commented Aug 30, 2016

MiguelMadero Aug 31, 2016

Choose a reason for hiding this comment

a15n Aug 31, 2016

Choose a reason for hiding this comment

a15n commented Sep 6, 2016

sir-dunxalot commented Sep 6, 2016

jamesdixon commented Sep 6, 2016

andrew-zenefits commented Sep 7, 2016

jliuzen Sep 7, 2016 • edited Loading

Choose a reason for hiding this comment

andrew-zenefits commented Sep 8, 2016 • edited Loading

jamesdixon commented Sep 16, 2016

andrew-zenefits commented Sep 24, 2016

sir-dunxalot commented Sep 24, 2016

a15n commented Sep 29, 2016

a15n commented Oct 3, 2016

sir-dunxalot commented Oct 5, 2016 • edited Loading

a15n commented Oct 5, 2016

sir-dunxalot commented Oct 5, 2016

sir-dunxalot commented Oct 6, 2016

a15n commented Aug 29, 2016 •

edited

Loading

a15n commented Aug 29, 2016 •

edited

Loading

a15n Aug 29, 2016 •

edited

Loading

a15n commented Aug 30, 2016 •

edited

Loading

jliuzen Sep 7, 2016 •

edited

Loading

andrew-zenefits commented Sep 8, 2016 •

edited

Loading

sir-dunxalot commented Oct 5, 2016 •

edited

Loading