Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add action href for map pins #149

Merged

Conversation

maks
Copy link
Contributor

@maks maks commented Jul 26, 2017

Add action for map pin selection

The action will be called when user selects a pin on the map. A new prop default_action controls whether the Maps default behaviour is called (showing ping annotation) after the jasonette action is run.

Also required small bug fix for "call" data property handling in JasonViewActivity.

Fixes: #144

maks added 3 commits July 26, 2017 13:24
consistent across call, success and error handlers
Action will be called when user selects a pin on the map. A new prop
default_action controls whether the GMaps default behaviour is called
(showing ping annotation) after the jasonette action is run.
@gliechtenstein
Copy link
Contributor

gliechtenstein commented Jul 28, 2017

I was testing this out but couldn't get it to work. Kind of confused because looking at the code looks like it's looking at the entire map component JSON to see if it has an action attribute (instead of on each pin)

Maybe I'm missing something? I was testing with https://jasonbase.com/things/LEED/edit but please feel free to share a JSON you tested with. Appreciate it!

@gliechtenstein
Copy link
Contributor

update: Just finished adding href/action on the ios side. Tested with https://jasonbase.com/things/LEED

commits:

@maks
Copy link
Contributor Author

maks commented Jul 29, 2017

@gliechtenstein sorry I spotted the issue right away, I put the action as a top level property of the map component, eg.

"sections" : [
  {
    "items" : [
      {
        "type": "map",
        "region": {
          "coord": "-37.907404, 145.069975",
          "width": "900",
          "height": "900"
        },
        "pins" : {
          "{{ #each $global.pins }}" : {
            "coord" : "{{ coord }}",
            "title" : "{{ title }}",
            "description" : "{{ description}}"
          }
        },
        "action" : {
          "trigger": "sel_pin"
        },
        "style": {
          "width": "100%",
          "height": "300"
        }
      }
    ]
  }
],

The reason I put it there was thinking of it in terms of the schema, the existing pins property is already an array of pin objects so didnt want to include a different object "type" in there.

And yes I did it for the entire map - the action will then pass in as parameters (ie. jasonette action options) the data of the pins selected, eg. my test action was:

"sel_pin" : {
    "type" : "$util.alert",
    "options" : {
      "title" : "pin data",
      "description": "Pin pressed! {{ JSON.stringify($jason) }}"
    }
  }

Surely doing an action per pin would get out of hand and normally you'd want just a generic action which lets you doing something with the data (title, coord, etc) of the selected pin.

Hope that makes sense?

@maks
Copy link
Contributor Author

maks commented Jul 29, 2017

I updated your example to work with this PR: https://jasonbase.com/things/1KKJ/edit

Sorry about the lack of documentation again, I should have put a more detailed description in the commit.
I tried to follow what existing actions/components used so for the map pin selected action, the options passed in are:

$jason.title - title of the pin
$jason.coord - coordinates in same lat,lng format as for $geo
$jason.description - description of the pin, in GMaps API this is called the snippet

@gliechtenstein
Copy link
Contributor

@maks thanks for the clarification. But wouldn't this limit us to only trigger static actions? For example, maybe we want each pin to have different actions. For example one pin can trigger an alert, another pin can trigger href, and another pin could trigger a $audio.play.

I do agree with you on the issue of schema/consistency though, and it would theoretically be nice if we could keep going this way with each component only having a single option, but I think this has only been possible because currently we only have simple components, and it won't scale.

This is especially true when we consider that we will officially support component extensions in the future, there can be much more complex components one may need to use.

In fact the map component is a great case for us to start thinking about this issue. I only discussed the pins here, but imagine having other components encapsulated within a component.

For example the map could come with a search input embedded. And this search input may have a completely different action compared to pins

mapsearch

Or we could have a custom callout (currently we just use the basic default callout via title and description attributes but we can theoretically use the existing Jasonette layout engine to implement the callout)

customcallout

In this case, the callout contains multiple buttons, so each button needs to have its own action attached.

Would love to hear your thoughts.

@maks
Copy link
Contributor Author

maks commented Jul 29, 2017

@gliechtenstein thats a very good point! Yes definitely need to allow future functionality.
Maybe we can put it in a object itself? eg.

{
...
  "actions" : {
     "pin_selected" : {
     } 
  }
}

this then allows for any future map features adding other named actions?

With having separate actions per pin, the issue becomes having to assign them. In the example I created, the pins come via separate json data - how would I be able to assign a action per each pin?

@gliechtenstein
Copy link
Contributor

In case of iOS I store the JSON on the view itself. I do that by overriding the base object with an additional attribute called payload Jasonette/JASONETTE-iOS@14a84f2#diff-09868d302d3fe7ad616179117c08ae0fR12 (NSObject is the most base level object in objective-c)

I find this trick useful for many purposes, and I use the same trick on Android here and there. For example. I use setTag https://github.com/Jasonette/JASONETTE-Android/blob/develop/app/src/main/java/com/jasonette/seed/Component/JasonComponent.java#L25 to set the JSONObject on a view, and use getTag to retrieve it later https://github.com/Jasonette/JASONETTE-Android/blob/develop/app/src/main/java/com/jasonette/seed/Component/JasonComponent.java#L160

Maybe we could try a similar approach here?

@gliechtenstein
Copy link
Contributor

BTW here's an updated demo with multiple pins https://jasonbase.com/things/Dzzo

@maks
Copy link
Contributor Author

maks commented Jul 31, 2017

@gliechtenstein I've updated PR per our slack discussion to use 1 action per pin. Now that you have the iOS implementation done too, I'll try to get another PR in for documentation repo updated too.

@gliechtenstein
Copy link
Contributor

@maks awesome, i just tried it out but this works a bit differently than I thought. It looks like the action gets triggered when you tap on the pin itself, which got me thinking about things I haven't thought of before.

The gist is, on iOS I've implemented it so that the action gets triggered when you tap on the callout (the bubble with title and description) instead of the pin. I just did it without thinking much because on iOS that's how it works by default. Normally on iOS when you tap the pin, the default action is it shows up the callout, and then the developer can implement a handler for tapping the callout.

I don't think there's a clear right or wrong solution for this (handling tap on pin vs. handling tap on callout) but i'm slightly tilting towards the callout option because normally people would want to see the callout before deciding to take action. If we attach action directly on the pin, then users have no way to view the callout before taking action.

What do you think?

@maks
Copy link
Contributor Author

maks commented Aug 1, 2017

@gliechtenstein very good point! I didn't realise that was the normal UX for map pins. I actually put in code to allow choosing whether or not to override the default "onclick" action for pins (displaying the "callout", GMaps api calls them "infowindow") or not.
I've added a commit that changes to this UX instead. One question I have is about the callout: I hide it after I call the action, do you do that too? I can see pro/con to both hiding it or leaving it showing.

@gliechtenstein
Copy link
Contributor

@maks thanks! In my case I just followed what some popular apps do on iOS. In case of yelp when you tap the callout it opens a new view, but when you come back, the bubble is still there, so you can keep track of where you last clicked, which prevents from redundant tapping.

callout

This UX instead of action being on the initial pin click, so user sees
the infowindow for the pin and needs to lick on it to trigger the action.
@maks maks force-pushed the 144-add-action-href-for-map-pins branch from e616edd to fd6aa0d Compare August 1, 2017 00:34
@maks
Copy link
Contributor Author

maks commented Aug 1, 2017

Good point! I've just ammended and force pushed a new commit here that doesnt hide the callout after the action.

@gliechtenstein gliechtenstein merged commit 685189f into Jasonette:develop Aug 1, 2017
@gliechtenstein
Copy link
Contributor

Awesome, just merged. Also thank you for refactoring some of the code! Just need some documentation and I think we can roll it out officially 🎉

@maks maks deleted the 144-add-action-href-for-map-pins branch August 7, 2017 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add action and href to map component pins
2 participants