feat: make possible for the service to specify the size of the intent modale #175

doubleface · 2017-06-12T10:36:10Z

The service can make a call to service to set the size of its modale :

cozy.client.intents.createService(intent, window)
.then(service => {
  service.setSize({width: '400px', height: '400px'})
})

The service is responsible to make the modale displayable on screen in different possible resolutions (mobile, desktop). The client application requesting the intent is still responsible for the display of the modal and the default size of it, if the service does not request any special modale size.

CPatchane

Seems good to me 👍

y-lohse · 2017-06-12T15:48:26Z

Can't this be achieved by styling the iframe element via css?

doubleface · 2017-06-12T16:03:16Z

@y-lohse We are styling the element wrapping the iframe which is the one showing the modale in the case of the onboarding at least.
The goal here is to let to the service called by the intent, which is the only one to know the content of it, the initiative to propose a better dimension for intent modal.
We tried to automate it at cozy-client-js level but the main document does not have access to the iframe content due to different subdomains.

y-lohse · 2017-06-12T16:11:41Z

Ah, I see. That's interesting, and not really something we considered when we did the spec. I'm taking note of it :)

doubleface · 2017-06-12T16:14:58Z

I think this may open a way to personalization for intent modals later, but restricted the list of authorized css properties at the moment to no wory too much.

gregorylegarec · 2017-06-12T23:53:54Z

Actually I feel that the way to do it is too much complicated. I suggest to do something like the following code, but maybe I am missing a point and it is not possible:

cozy.client.intents.createService(intent, window, {
  dimensions: {
    with: 400,
    height: 400
  }
})

1/ The signature of cozy.client.intents.createService() should also become cozy.client.intents.createService(intent, window, options). options could be passed to the client directly at the handshake (So client send data, service send options). The main benefit is to avoid creating a new method setSize() in the returned service object, with a dedicated postMessage message. It is also future-ready when we will need, for sure, to pass new informations to client.

2/ As the cozy.client.intents.createService() now detects directly intent ID in URL and window object, we can change its signature further without introducing regressions in existing code. The final signature should be: cozy.client.intents.createService(options). And we may also force intent or window by passing them as optionnal parameters options.intent and options.window. Maybe this part needs a preparative PR.

3/ Also I think we should not pass a string because the integer values cannot be used without being parsed, and maybe the client will not use it directly for styling with CSS. If we give an integer value directly, it is still easy to generate a string like '400px' client-side.

4/ Did you try to pass an element instead dimensions and let cozy-client detect its width and height ?

What do you think @doubleface, @y-lohse ?

y-lohse · 2017-06-13T07:37:18Z

Regarding 1 & 2 : I'm always a bit cautious when it comes to a options parameter that can take all sorts of values, since they are generally hard to document and grasp. However, since this API is evolving to actually support many optional parameters, I think it may be justified. And it would be more future proof than adding a new method.

Yes, I agree.
Not sure I understood that part :)

doubleface · 2017-06-13T07:44:45Z

@gregorylegarec
1/ I agree with you, that this signature is far cleaner, and this was actually my first idea. But in the case of the create account intent, we have to change the dimensions of the modal once we now if the connector is oauth or not, and at the moment of the createService call, we only have the connector's slug. Nonetheless, your proposition would work and would be better in most cases, but only as a shortcut for my proposition. I don't see a way to avoid the postMessage at the moment.
2/ I like it but would like to know @y-lohse advice also
3/ My idea was to pass strings to allow services to use the unit they want. I find it more flexible. And the idea was also to allow more customization in the future by allowing more css properties to be changed (but I the setSize is not future proof in that way).
4/ I tried it, and failed to pass element to the parent window but I didn't see I could calculate the size of the element in cozy-client-js in the iframe. I will do it, thanks @gregorylegarec .

gregorylegarec · 2017-06-13T15:41:25Z

@doubleface Thanks, you're right I did not get the fact that we needed the connector before being able to first. So yes, we have to use a new method in the service object. 👍

However I still have a doubt about passing to much specificity about the size or units. For several reasons:

Which unit pass ? em ? It can't work I think. The service does not know what is the reference for the font size.
It is not the service's goal to redesign the client.
For now we just need to pass integer values in px. Experience told me to never develop more than what is needed.

I am going to look at the code.

doubleface · 2017-06-13T15:46:47Z

@gregorylegarec I think I understand your point. The fact is that I only pass px for the account intent anyway and the dom property only generates size with px also. Then it would be coherent.

Then ok, let's go for the px unit. It is still possible to extend it easily in the futur anyway.

gregorylegarec

Sorry, I am a little late, the whole thing is really good but I have some micro-feedback (naming and so), I'll end my review tonight.

gregorylegarec · 2017-06-13T16:11:07Z

src/intents.js

@@ -46,6 +46,14 @@ function injectService (url, element, intent, data) {
        return event.source.postMessage(data, event.origin)
      }

+      if (handshaken && event.data.type === `intent-${intent._id}:size`) {
+        ['width', 'height', 'maxWidth', 'maxHeight', 'minWidth', 'minHeight'].forEach(prop => {


I think we should not care about properties which we don't use or need for now, so keep only width and height. It is more code to maintain and to keep tested. What do you think ? What are @CPatchane and @y-lohse opinions about this point ?

I don't really mind it tbh. I can see how they would be useful even in the current use case.

I think it could be useful for later without adding complications here so I would prefer to keep it 👍

I totally disagree because we will have code which is not used, but if I am alone let's go with it. (Just brace yourself for the day someone will ask "May I change maxHeight ? Where is it used ? What the difference with height ? 😄 )

Then @gregorylegarec you will have the right to laugh at us if that may comfort you a little 😄

After thinking @doubleface, I can't see any cases when we will need max and min css properties here. I was first thinking about responsive behaviour but, even that, if we use the service from cozy-collect, the app which handle the inter-app view will the provide a responsive width and height. So I think we would just need width and height here. What do you think? And @y-lohse ?

If we're allowing the service to control the size of it's container, I think min and max sizing are a valid part of that and are ok here. But ultimately I haven't seen the mockups or anything, so it's hard for me to say if they will ever be used or not. You guys are working this app, you know this better than me :)

@CPatchane I actually use maxHeight and maxWidth in https://github.com/doubleface/cozy-client-js/blob/389578a203c7152e9e239b55db2a6e3843c852ad/src/intents.js#L158 since it allows the modale to adapt it's size if the resolution lower than expected.

Oh, I didn't see it like that, so we just need the max properties? If we don't need the min properties we can skip them for now. It's just better to not handle not used yet properties to avoid useless maintenance 👍

Then let's just remove min properties and I merge

gregorylegarec · 2017-06-13T16:12:41Z

src/intents.js

+          if (event.data.document[prop]) element.style[prop] = event.data.document[prop]
+        })
+
+        return true


As it is just an early return, I think we should not return true or another value. When we read this part, it feels that the true value has some kind of importance, but it is not the case.

gregorylegarec · 2017-06-13T16:15:35Z

src/intents.js

+        if (terminated) throw new Error('Intent service has already been terminated')
+
+        // if a dom element is passed, calculate its size and convert it in css properties
+        if (message.document.dom) {


By renaming the dom attribute to element, we can avoid the previous comment.

I was actually hesitating between dom and element

gregorylegarec · 2017-06-13T17:35:04Z

src/intents.js

@@ -142,6 +150,19 @@ export function createService (cozy, intentId, serviceWindow) {
        serviceWindow.parent.postMessage(message, intent.attributes.client)
      }

+      const setSize = (message) => {
+        if (terminated) throw new Error('Intent service has already been terminated')


s/has already been/has been/

gregorylegarec

I am really sorry, a lot of comments, much about tiny details. I feel like an evil perfectionnist so please @CPatchane and @y-lohse give us your opinion about my comments. This PR needs discussion because what is done in Cozy Client is done for a looong time, if not forever. 😄

gregorylegarec · 2017-06-13T22:38:12Z

src/intents.js

@@ -165,6 +186,10 @@ export function createService (cozy, intentId, serviceWindow) {
              type: `intent-${intent._id}:error`,
              error: errorSerializer.serialize(error)
            }),
+            setSize: (doc) => setSize({


setSize() signature should not have a document as parameter, but something like dimensions.

I agree with that one, dimensions is more explicit.

gregorylegarec · 2017-06-13T22:47:19Z

src/intents.js

@@ -46,6 +46,14 @@ function injectService (url, element, intent, data) {
        return event.source.postMessage(data, event.origin)
      }

+      if (handshaken && event.data.type === `intent-${intent._id}:size`) {
+        ['width', 'height', 'maxWidth', 'maxHeight', 'minWidth', 'minHeight'].forEach(prop => {
+          if (event.data.document[prop]) element.style[prop] = event.data.document[prop]


Ok now I understand what is done here and why CSS values are directly transmitted. Again, I am not a big fan of letting the service do whatever he wants with client's iframe CSS values. It should limit its role to transmit width and height. Nothing more is needed. Again, if @CPatchane and @y-lohse agree with this, let's keep it.

Summary of what I suggest:

['width', 'height'].forEach(property => { element.style[property] = `${event.data.dimensions[property]}px` || element.style[property] }

It is reeeeaaaally difficult for me to consider passing directly a CSS value as a string containing px, sorry about that.

gregorylegarec · 2017-06-13T22:50:23Z

src/intents.js

@@ -165,6 +186,10 @@ export function createService (cozy, intentId, serviceWindow) {
              type: `intent-${intent._id}:error`,
              error: errorSerializer.serialize(error)
            }),
+            setSize: (doc) => setSize({


The more I think about this method name the more I feel that it is not explicit enough. Calling intentService.setSize() give the idea that we are setting the service own size. I suggest something like setClientDimensions(dimensions) or resizeClient(dimensions). What's your opinion about that ?

I did not like setSize a lot. Let's go for resizeClient.

… modale

This dom propertie allows to automatically adapt the size of the modal to a given dom element

gregorylegarec · 2017-06-15T14:27:04Z

src/intents.js

@@ -137,11 +145,24 @@ export function createService (cozy, intentId, serviceWindow) {
      let terminated = false

      const terminate = (message) => {
-        if (terminated) throw new Error('Intent service has already been terminated')


My remark was just for resizeClient(): it is the only method which does not terminate the service.

doubleface requested a review from gregorylegarec June 12, 2017 10:36

doubleface changed the title ~~feat: make possible for the service to specify the size of the intent modale~~ [WIP] feat: make possible for the service to specify the size of the intent modale Jun 12, 2017

doubleface requested review from CPatchane, m4dz and lespacedunmatin June 12, 2017 13:28

doubleface self-assigned this Jun 12, 2017

lespacedunmatin approved these changes Jun 12, 2017

View reviewed changes

m4dz approved these changes Jun 12, 2017

View reviewed changes

CPatchane approved these changes Jun 12, 2017

View reviewed changes

doubleface requested a review from y-lohse June 12, 2017 16:06

doubleface changed the title ~~[WIP] feat: make possible for the service to specify the size of the intent modale~~ feat: make possible for the service to specify the size of the intent modale Jun 13, 2017

gregorylegarec reviewed Jun 13, 2017

View reviewed changes

doubleface added 7 commits June 14, 2017 16:39

feat: make possible for the service to specify the size of the intent…

a4e7c76

… modale

feat: allow more css properties to be defined in setSize

9831f2c

feat: add dom property to the setSize method

e085a23

This dom propertie allows to automatically adapt the size of the modal to a given dom element

doc: setSize method documentation

b9b84be

fix: various renamings after review

ddf0981

test: added specific unit tests

389578a

fix: removed unused minHeight and minWidth properties

646bbca

doubleface merged commit cc46436 into cozy:master Jun 15, 2017

gregorylegarec reviewed Jun 15, 2017

View reviewed changes

y-lohse mentioned this pull request Aug 28, 2017

Evolving intents #201

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make possible for the service to specify the size of the intent modale #175

feat: make possible for the service to specify the size of the intent modale #175

doubleface commented Jun 12, 2017 •

edited

CPatchane left a comment

y-lohse commented Jun 12, 2017

doubleface commented Jun 12, 2017

y-lohse commented Jun 12, 2017

doubleface commented Jun 12, 2017 •

edited

gregorylegarec commented Jun 12, 2017 •

edited

y-lohse commented Jun 13, 2017

doubleface commented Jun 13, 2017

gregorylegarec commented Jun 13, 2017

doubleface commented Jun 13, 2017

gregorylegarec left a comment

gregorylegarec Jun 13, 2017

y-lohse Jun 13, 2017

CPatchane Jun 13, 2017

gregorylegarec Jun 13, 2017

doubleface Jun 13, 2017

CPatchane Jun 14, 2017

y-lohse Jun 14, 2017

doubleface Jun 15, 2017

CPatchane Jun 15, 2017

doubleface Jun 15, 2017

gregorylegarec Jun 13, 2017

gregorylegarec Jun 13, 2017

doubleface Jun 13, 2017

gregorylegarec Jun 13, 2017

gregorylegarec left a comment •

edited

gregorylegarec Jun 13, 2017 •

edited

doubleface Jun 13, 2017

gregorylegarec Jun 13, 2017

gregorylegarec Jun 13, 2017

doubleface Jun 13, 2017

gregorylegarec Jun 15, 2017

feat: make possible for the service to specify the size of the intent modale #175

feat: make possible for the service to specify the size of the intent modale #175

Conversation

doubleface commented Jun 12, 2017 • edited

CPatchane left a comment

Choose a reason for hiding this comment

y-lohse commented Jun 12, 2017

doubleface commented Jun 12, 2017

y-lohse commented Jun 12, 2017

doubleface commented Jun 12, 2017 • edited

gregorylegarec commented Jun 12, 2017 • edited

y-lohse commented Jun 13, 2017

doubleface commented Jun 13, 2017

gregorylegarec commented Jun 13, 2017

doubleface commented Jun 13, 2017

gregorylegarec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregorylegarec left a comment • edited

Choose a reason for hiding this comment

gregorylegarec Jun 13, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doubleface commented Jun 12, 2017 •

edited

doubleface commented Jun 12, 2017 •

edited

gregorylegarec commented Jun 12, 2017 •

edited

gregorylegarec left a comment •

edited

gregorylegarec Jun 13, 2017 •

edited