-
Notifications
You must be signed in to change notification settings - Fork 27
Update kept to work with the new daybed.js #26
base: daybed_storage
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this operation idempotent ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is, yes, but I don't understand what's the matter here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you compare the current value then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This says: "If the current session is different from the one of the hash, then set the hash to the new value".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean : what's the problem with overwriting with the same value ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I prefer avoid doing so as I don't know what that triggers for the browser (e.g. if there is no need to, why should we update it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, no problem dude ! Just FYI onhashchange is called only if the value changes :)
src/js/store/daybed.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this : each user will have its own model ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each token, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explicit pros and cons of this pattern please ? Maybe in Daybed documentation page about permissions ?
( only one I can foresee is about managing permissions, but TBH I find it weird )
|
@n1k0 do you know if the failures are due to the daybed.js changes or aren't related? |
src/js/store/daybed.js
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might have to choose a way to indent then() consistently...
I think I prefer this way:
return Daybed.startSession(self._options.host, {
token: this._options.token
})
.then(function(session) {
// ...
});
This uses the new daybed.js API and works with kept to share the dashboards, using a hash in the url.