improve hover behavior, simplify interface, and improve readme #68
Conversation
gkchestertron
requested review from
notmessenger,
sophypal,
juwara0,
TheOtherDude and
quincyle
|
Changes Unknown when pulling a44778b on gkchestertron:improve-popover-hover into ** on ciena-frost:master**. |
README.md
Outdated
|
|
||
| ## Specifying Target | ||
|
|
||
| If the `frost-popover` component is placed next to the `target`, be careful to use a selector that will uniquely | ||
| identify the `target`. If it is nested inside the `target`, you can set `closest` to true which will search the | ||
| nearest ancestor from the `popover`. | ||
| nearest ancestor from the `popover` - which is far more performant than a full dom traversal. |
There was a problem hiding this comment.
Comment is needless in my opinion. Unless we're talking about huge perf gains, comments like these sound like marketing BS. It'll also bias our users to always using the nested version which is not what I want. I'm actually a fan of the sibling approach as the positioning is much more predictable. The nested version has it's merits and it's the only way to have unique popovers for arrays of similar items.
There was a problem hiding this comment.
Those kinds of traversals can become real problems - but I suppose here they can only be kicked off as fast as a user can hover their mouse. And, I agree - I like the sibling approach better. The includeContentInEvents flag didn't work with the sibling approach, which is what led me to needing to work on this.
There was a problem hiding this comment.
Most of jQuery can become real problems :) In a majority of cases, I try not to optimize unless I know the code is running "hot" or executed many times. Since the popover getTarget didn't meet that criteria, it wasn't a cause of concern for me and so not a concern I want to inform users about.
| @@ -22,7 +22,6 @@ export default Component.extend(PropTypeMixin, { | |||
| excludePadding: PropTypes.bool, | |||
| handlerIn: PropTypes.string, | |||
| handlerOut: PropTypes.string, | |||
| includeContentInEvents: PropTypes.bool, // making this true lets the content also inheret events properly | |||
There was a problem hiding this comment.
What's the reason for removing this. Wouldn't it be considered a #MAJOR# if you removed a feature.
There was a problem hiding this comment.
So, normally yes. But this was added just for one feature in mcp recently, and was not documented. So, no one else is using it, and now mcp isn't either. And the functionality is brought forward - but you can't turn it off. If we feel this should be behind a flag, we can do that, but it prob shouldn't be called this. And I don't think it should be something you can turn off - when would you specifically not want to be able to hover/select the text in a popover?
There was a problem hiding this comment.
I think @EWhite613 may have added this? Even if this flag isn't used in MCP, it might be used in other apps. Let's be on the safe side and make this a #MAJOR#.
addon/components/frost-popover.js
Outdated
| @@ -142,6 +143,21 @@ export default Component.extend(PropTypeMixin, { | |||
|
|
|||
| $(target).on(event, this._eventHandler) | |||
| } | |||
|
|
|||
| // handle mouse events on visible popover | |||
| $(popover).on('mouseenter', event => { | |||
There was a problem hiding this comment.
Might be a good idea to use off when the component dies. Even if popover is destroyed when the component dies, the event listeners are still attached due to how Ember destroys the DOM.
There was a problem hiding this comment.
Umm... yes. A really good idea. Thank you.
addon/components/frost-popover.js
Outdated
| @@ -164,11 +180,7 @@ export default Component.extend(PropTypeMixin, { | |||
| * @param {DOMEvent} event - click event | |||
| */ | |||
| togglePopover (event) { | |||
| const popover = this.get('element') | |||
| if ($(event.target).closest(popover).length === 0 || | |||
There was a problem hiding this comment.
So does this remove the ability to have a sustained popup and clicking on the outside to close it?
There was a problem hiding this comment.
Actually, no - this still works because I am blocking the click on the content from propagating. But, what does happen is that if the popover is meant to work on click and you hover over it - it will now close on mouseleave.
|
Changes Unknown when pulling 92b7f48 on gkchestertron:improve-popover-hover into ** on ciena-frost:master**. |
|
👍 |
This project uses semver, please check the scope of this pr:
CHANGELOG