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

Intents improvements (onReadyCallback, exposeFrameRemoval) #189

Merged
merged 7 commits into from
Jul 20, 2017

Conversation

CPatchane
Copy link
Contributor

@CPatchane CPatchane commented Jul 19, 2017

Two main points:

  • Handle exposeIntentFrameRemoval data flag to terminate the service without removing the intent DOM node directly but by providing the removal function to the client
    Example of usage: Claudy has an animated (CSS) menu closing. Using this feature it will be possible to terminate the intent .then listen the animation ending before removing the intent iframe node using the resolved removeIntentFrame function (and avoiding a sudden blank view in the Claudy view)

  • Add an onReadyCallback optional argument to the create method in order to allow providing a callback function to be run when the intent iframe will be loaded (iframe onload listener).
    Example of usage: Claudy will now wait the intent iframe to be totally loaded before beginning its opening animation in order to keep having a fluid UX.

Edit: Rename exposeRemoving to exposeFrameRemoval and use a data flag instead of a method

@y-lohse
Copy link
Contributor

y-lohse commented Jul 20, 2017

  • onReadyCallback : no problem, pretty straightforward
  • exposeRemoving : this one took me a while to figure out how it gets used, but it's ok now. I think removeIntentFrame is a good name, but exposeRemoving not really. Maybe something like exposeFrameRemoval or manuallyRemoveFrame or whatever? exposeRemoving is not very clear I think.

So API-design wise that's all, the code is good too. I know you plan to update the changelog, but could you please also update the intent documentation?

@CPatchane
Copy link
Contributor Author

CPatchane commented Jul 20, 2017

Thanks @y-lohse! Sure, I was hesitating about the exposeRemoving name and your proposals sound better so I will change that 👍
And of course update the docs :)

@CPatchane CPatchane force-pushed the improve_intent branch 2 times, most recently from e6c75a3 to 64ff1f6 Compare July 20, 2017 10:29
@CPatchane CPatchane changed the title [WIP] Intents improvements Intents improvements (onReadyCallback, exposeFrameRemoval) Jul 20, 2017
@y-lohse
Copy link
Contributor

y-lohse commented Jul 20, 2017

The changes are excellent! However I'm afraid I have a new question (just thought about it, sorry).
Right now, the service decides whether or not the iframe gets closed automatically, or if the client has to do it. This means clients must check for a removeIntentFrame property all the time, because they can't know if the service will do it or not. Is this correct? Have you considered doing it the other way around, ie. the client says "Please don't remove the frame when you're done"? Would that make sense?

@CPatchane
Copy link
Contributor Author

Hum, you're right, I'll think about that to be simpler and less error prone 👍

@y-lohse
Copy link
Contributor

y-lohse commented Jul 20, 2017

Ok, cool. Sorry again I didn't mention it during the first review, the thought only occured to me later :/

@CPatchane
Copy link
Contributor Author

CPatchane commented Jul 20, 2017

@y-lohse That was a very very good point in fact! Better late than never^^ Now it's even better, if the intent find a boolean exposeIntentFrameRemoval in its data provided by the client, the terminate() method will return the removal function with the doc autmatically, no need to use a different method in the service 🎸🎸
See here 3f56fb8

Copy link
Contributor

@y-lohse y-lohse left a comment

Choose a reason for hiding this comment

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

Great! I think this API makes sense. Ready to ship as far as I am concerned!

@CPatchane
Copy link
Contributor Author

Thanks @y-lohse 🎉

@CPatchane CPatchane merged commit 01bf933 into cozy:master Jul 20, 2017
@CPatchane
Copy link
Contributor Author

@y-lohse y-lohse mentioned this pull request Aug 28, 2017
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.

None yet

2 participants