Skip to content

Conversation

@tommydarko
Copy link
Contributor

@tommydarko tommydarko commented Jul 14, 2016

For your consideration. Working examples of plugin can be seen here:

  1. With an embedded experience: http://demo.ceros.com/examples/toms-tests/state-of-the-layer/#animal=cow
  2. With a stand alone experience : http://view.ceros.com/labs-tom-barretts-tests/example-2-1/p/1#animal=chicken

README.md Outdated

## The State of the Layer Plugin

Allows people using the Ceros Studio's SDK pallet to tag components to enable them to both control, and be controlled by, parameters in the Experience's URL. It can be used to remember user's actions between pages or to trigger actions like showing layers or jumping to anchor points when the Experience is opened by the user.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example of what it is to "remember actions between pages"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to just barge into this code review but I'd really like a concrete use case not accomplishable with the current functionality of Ceros and the SDK that this plugin enables. I am very confused about what this plugin does.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addelong i thought all plugins are built up on top of the SDK and functionality of ceros so no plugins accomplish more than ceros and the sdk i thought

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean like, I am confused about what this is actually doing, and is there a different way of accomplishing what we want that's already built in to Ceros.

Just looking for a use case to help me understand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think of it in terms of a single page experience with 5 tabs. With normal Ceros, if the experience is refreshed, it goes back to the original tab open. With plugin improved Ceros, when you click a tab it updates the URL. Now when you refresh, the layers are set so that the tab you clicked is open by default.

Copy link
Contributor

@SamJacobs SamJacobs Jul 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its essentially providing a framework for using the # as a means to click hotspots across page requests.

basically I create a hotspot that does some thing. tag it with "now-kiss=true".

then i can go to <viewurl>#now-kiss=true and that hotspot will get clicked.

tom has provided a way to use set-state to append the payload of a component to the url to give some interaction that will i suppose allow for a deep link to a non page load state of an experience etc etc

Tom might have to explain where the need comes from

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aaaaahhhhhh, I see.

I would like to vote for a name change for this plugin, might I suggest #doThings

@SamJacobs
Copy link
Contributor

I believe you also need to make a couple changes to the gruntfile in the root of the repo @tommydarko

@SamJacobs
Copy link
Contributor

Search for 'eloqua' and you can see a bunch of places that need sections added for new plugins and a string in a list of plugins

"use strict";

/**
* Class to mange state represented in the URL
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mange!

@SamJacobs
Copy link
Contributor

@tommydarko done with first pass through ->
I think it would be super helpful if chunks of logic were extracted and variables renamed and mapped to the man things the plugin is doing as you explained it to me which i found super helpful!

  1. there are tagged/named components that trigger some action configured in the studio
  2. Objects with payload will have the payload added to the url
  3. on player page change we parse the hash and then trigger the action in step 1

Note: changing the hash currently has no effect on the current page, it is only parsed the next time a page change event is emitted.

@SamJacobs SamJacobs assigned tommydarko and unassigned SamJacobs Jul 15, 2016
@tommydarko
Copy link
Contributor Author

Hey @SamJacobs: Please could you take another look through? Thank you! :-)

@tommydarko tommydarko changed the title v0.1.0 of The State of the Layer Plugin v0.1.0 of The URL Params Plugin (Previously known as The State of the Layer Plugin) Jul 19, 2016
@SamJacobs SamJacobs changed the title v0.1.0 of The URL Params Plugin (Previously known as The State of the Layer Plugin) v0.1.0 of The State of the Layer Plugin Jul 20, 2016
README.md Outdated
3. Use the SDK Inspector Pallet to tag the hot spot with a name and a value to be used as the URL parameter. For example if you wanted to show the second slide in a slide show, you could tag it `slide=2` - this would allow you to trigger this action by adding `#side=2` to the end of the URL.

4. If you would like to be able to update the URL based on user's actions in the Studio, tag components with `set-state` and set the pay load to the name/value pair you would like to add to the URL.
3. Use the SDK Inspector Pallet to tag the hot spot with a name and a value to be used as the URL parameter. For example if you wanted to show the second slide in a slide show, you could tag it `slide=2` - this would allow you to trigger this action by adding `side=2` to the end of the URL, after a `#` symbol.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry the note on this was a typo in side=2

@SamJacobs
Copy link
Contributor

@tommydarko i made some more notes, ready for you

@SamJacobs SamJacobs removed their assignment Jul 20, 2016
@tommydarko
Copy link
Contributor Author

@SamJacobs: please can you take another pass? Hopefully for the last time! :-)

setParameterValueForKey: function(theKey, theValue) {

// If we have this key already
if (this.hasKey(theKey)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh sorry tom, one thing i did notice is that we loop through the state for has key, then loop through again to find the right one.

its probably not a huge deal because the size of state is very small, but i dont know if you need the haskey method. essentially you loop through them all, if the key of the current index is the key you want to set then set it and return, otherwise if you get all the way through the looop without returning you make a new object and push it onto state.

just a little optimization, really not necessary, ill give a +1 without it, but let me know if you want to make this change and i'll re +1

@SamJacobs
Copy link
Contributor

code 👍

one little note but feel free to ignore it, i dont think it's essential

@SamJacobs
Copy link
Contributor

oh also change the name of the issue/pull request to url params, the branch can stay the same prolly

@SamJacobs SamJacobs added testing and removed review labels Jul 21, 2016
@tommydarko tommydarko changed the title v0.1.0 of The State of the Layer Plugin v0.1.0 of The URL Params Plugin Jul 21, 2016
@qualityshepherd
Copy link

Tom/Sam does this need testing?

@grahamedaley
Copy link

@tommydarko @SamJacobs does this need testing?

@SamJacobs
Copy link
Contributor

this is tom's PR, i just reviewed it

@brianpabent
Copy link
Contributor

Is this one that @tommydarko can release through his own QA process? Perhaps it is already released?

@grahamedaley
Copy link

@tommydarko does this require QA to test? if so this could be something you could pick up @qualityshepherd ?

@danwashable
Copy link
Contributor

In response to @brianpabent's question, this does not need to go through our process. @tommydarko can move this through his own process. We don't need to worry about it.

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.

9 participants