Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add public showAtEvent method for enyo.Popup #166

Merged
merged 2 commits into from Nov 28, 2012

Conversation

Projects
None yet
4 participants
Contributor

onecrayon commented Nov 8, 2012

I've been playing around with porting an app from Enyo 1 to Enyo 2, and was frustrated to find that enyo.Popup does not supply any positioning options for working with mouse events. So I wrote my own logic to mimic the behavior of Enyo 1's old openAtEvent method, and here it is.

Let me know if you prefer to receive changes that add features (as opposed to fixing bugs) via some other method. I figured I may as well just submit a pull request on it since the Contributing.md docs didn't mention anything special, but if there's a more thorough process for submission to core I can certainly go that route instead in the future.

@onecrayon onecrayon Add public showAtEvent method to enyo.Popup for easier positioning ne…
…ar mouse events

Enyo-DCO-1.1-Signed-off-by: Ian Beck <ian@onecrayon.com>
1262f0a
Contributor

unwiredben commented Nov 8, 2012

pull requests are fine, but you might also want to bring up the suggestion on the dev list, https://groups.google.com/forum/?fromgroups#!forum/enyo-development

@unwiredben unwiredben was assigned Nov 8, 2012

@unwiredben unwiredben and 2 others commented on an outdated diff Nov 13, 2012

source/ui/Popup.js
@@ -197,5 +256,33 @@ enyo.kind({
requestHide: function(inSender, inEvent) {
this.hide();
return true;
+ },
+
+ //* @public
+ /**
+ Open at the location of a mouse event (_inEvent_). The popup's
+ position is automatically constrained so that it does not
+ display outside the viewport.
+
+ _inOffset_ is an object which may contain left and top
+ properties to specify an offset relative to the location the
+ popup would otherwise be positioned.
+ */
+ showAtEvent: function(inEvent, inOffset) {
@unwiredben

unwiredben Nov 13, 2012

Contributor

I know the showAtEvent API was in 1.0, but I'm wondering if maybe this should be a "showAtPosition" with it decoupled from the event structure definition. Then you wouldn't need the inOffset either.

@mbessey

mbessey Nov 13, 2012

I would tend to agree. I think the idea behind showAtEvent was that most of the time, you're going to show a Popup in response to a tap event, so you can just pass the "inEvent" parameter from the ontap handler straight to showAtEvent.

@onecrayon

onecrayon Nov 14, 2012

Contributor

Implementing a "showAtPosition" option would be relatively trivial using the same logic provided by the "showAtEvent" code. The reason I went with showAtEvent was because:

  1. As mbessey noted, it's easier to simply pass through an inEvent object. To properly send things through to showAtPosition, I would need to grab the X/Y coordinates from the event object, which necessitates checking for which ones are supported in a given browser.
  2. It doesn't have to be this way, but at least to me "showAtPosition" sounds like it will position the Popup exactly at those coordinates, whether or not this causes it to overflow the viewport. Underlying this assumption for me was the Enyo 1.0 openAt function which allowed users to specify top/left/bottom/right coordinates, and had an explicit second parameter for opting into automatic repositioning.
  3. I only needed to personally use showAtEvent, and since I'm not responsible for making Enyo 2 a broadly-useful framework saw no reason to write in extra methods just on the off chance other people would find them useful. :-)

However, the underlying logic that drives showAtEvent is generic enough that you could easily implement showAtPosition to serve as the underlying general-purpose call. For instance:

showAtEvent: function(inEvent, inOffset) {
    // Calculate our ideal target based on the event position and offset
    var p = {
        left: inEvent.centerX || inEvent.clientX || inEvent.pageX,
        top: inEvent.centerY || inEvent.clientY || inEvent.pageY
    };
    if (inOffset) {
        p.left += inOffset.left || 0;
        p.top += inOffset.top || 0;
    }
    this.showAtPosition(p);
},

showAtPosition: function(inPosition) {
    // Save our target position for later processing
    this.targetPosition = inPosition;

    // Show the dialog
    this.show();
}

Of course, that would still have it repositioning automatically to ensure the Popup is in the viewport (repositioning logic is living in updatePosition in my code), but frankly I'm not sure my initial assumptions about that method (or the Enyo 1 design) make all that much practical sense, anyway.

@unwiredben

unwiredben Nov 14, 2012

Contributor

OK, I'm sold on the current code.

Contributor

unwiredben commented Nov 14, 2012

OK, can you also update the popup sample to show off this API? Then I'll pull this in (although probably right after we tag 2.1.1)

This works for me as a solution to popping up something above the event. I was trying to use onyx.Menu to popup above rather than below the item. This works fine instead.

Though, just FYI, if you use the method in onyx.Menu to display something above the Menu (say in a bottom toolbar) it wont' work that way (it disappears into the top of the toolbar).

For onyx.Menu I figured out a way to do it though.

Contributor

onecrayon commented Nov 15, 2012

I went looking for a Popup sample in the root samples directory, but couldn't find anything. Are you looking for a new Popup-specific sample, or is there an existing sample that I could update?

Also, would you like the showAtPosition code integrated as I outlined it above?

Contributor

unwiredben commented Nov 15, 2012

I guess the popup sample is in the onyx folder, not here. Adding a demonstration there would be sufficient. Look in the samples folder of that library.

@onecrayon onecrayon Add public showAtPosition method for enyo.Popup should people want to…
… explicitly locate the popup (or anchor to a specific corner)

Enyo-DCO-1.1-Signed-off-by: Ian Beck <ian@onecrayon.com>
1e32bc8
Contributor

onecrayon commented Nov 15, 2012

As you're doubtless aware, the sample pull request is submitted for the Onyx repo, and I added the showAtPosition option as discussed above to this branch.

Contributor

unwiredben commented Nov 26, 2012

I'm holding to add these two pull requests once 2.1.1 is tagged.

Contributor

onecrayon commented Nov 26, 2012

Yep, I figured! No rush.

@unwiredben unwiredben added a commit that referenced this pull request Nov 28, 2012

@unwiredben unwiredben Merge pull request #166 from onecrayon/popup-positioning
Add public showAtEvent method for enyo.Popup

Reviewed-By: Ben Combee (ben.combee@palm.com)
c8d5110

@unwiredben unwiredben merged commit c8d5110 into enyojs:master Nov 28, 2012

Contributor

unwiredben commented Nov 28, 2012

merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment