Skip to content
This repository has been archived by the owner on Aug 23, 2019. It is now read-only.

Tmyrden/add collapse memory #31

Closed
wants to merge 38 commits into from
Closed

Conversation

tmyrden
Copy link

@tmyrden tmyrden commented Dec 6, 2013

Closes #29

View initialize now calls back
Key is set when you toggle the collapse
Key is used to set initial toggle status

var tasks = [
this._view.initialize,
_.bind(this._userCache.initialize, this._userCache),

Choose a reason for hiding this comment

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

No need to do the bindAll in the constructor just for view#initialize. Can just bind view#initialize in the same way as the usercache#initialize here.

this._wrapper.parentNode.removeChild(this._wrapper);
}
if (this._wrapper) {

Choose a reason for hiding this comment

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

spacing between the if statements

Chat.prototype._collapseClick = function() {
var userKey = this._userCache.getLocalUserKey();
this._view.toggleCollapse();
userKey.key(WIDGET_NAMESPACE).key('collapsed').set(this._view._collapsed);

Choose a reason for hiding this comment

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

if view._collapsed is being used publicly it should just be view.collapsed. Or just leave it _.collapsed and add a getter method in view.

@@ -38,7 +38,7 @@ var VALID_POSITIONS = ['left', 'right'];

var defaultOpts = {
room: null,
collapsed: false,

Choose a reason for hiding this comment

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

Can just leave this to the default false and do your check in the view the other way around. The default should be left as false, because that is still the default when you load up chat for the first time.

Copy link
Author

Choose a reason for hiding this comment

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

It's not anymore. Look at the top of the index.js file. Default is null, otherwise I have no way to check that collapsed param wasn't set by the user. Thats what the whole _.isBoolean(this.collapsed) check is for.

Choose a reason for hiding this comment

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

The logic here can be done the other way around:

// If the collapsed param is specified, we use it, otherwise

first check for the collpased param on the userObj, if it does not exist use the default of false

Choose a reason for hiding this comment

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

disregard the above. It is implemented as intended.

@@ -88,6 +88,7 @@ module.exports = Chat;

this._room = validOpts.room;
this._messageExpiry = validOpts.messageExpiry;
this._collapseKey = this._room.self().key(WIDGET_NAMESPACE).key('collapsed');

Choose a reason for hiding this comment

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

unused member var

@tmyrden tmyrden closed this Dec 9, 2013
@tmyrden tmyrden deleted the tmyrden/add-collapse-memory branch December 9, 2013 15:50
@tmyrden tmyrden restored the tmyrden/add-collapse-memory branch December 9, 2013 20:29
@tmyrden tmyrden deleted the tmyrden/add-collapse-memory branch December 9, 2013 22:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Collapse Tracking
2 participants