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

Improved scroll performance #9

Merged
merged 10 commits into from
Jun 24, 2014
Merged

Conversation

mspensieri
Copy link
Contributor

  • Properly reuse tableview cells
  • Extracted 3 subclasses from SOMessageCell - SOPhotoMessageCell, SOTextMessageCell, SOVideoMessageCell.

@VincentSit
Copy link

Looks like the image of play button can not customize?

@VincentSit
Copy link

More clearly than before, but why not use the class cluster design patterns? I think that should be better.

@mspensieri
Copy link
Contributor Author

@VincentSit The play button was never customizable, I did not change this. I agree it would be a nice feature though :)

As for the class cluster pattern, I don't think it's appropriate in this case since the subclasses should not be transparent to the user. Each subclass has properties that the base class should not have (ex: the PhotoMessageCell has a 'mediaImageView' property. It doesn't make sense to expose this to the base class since it does not exist in the TextMessageCell class)

@VincentSit
Copy link

Thanks for the explanation.

arturdev added a commit that referenced this pull request Jun 24, 2014
Improved scroll performance
@arturdev arturdev merged commit 4d4945b into matghazaryan:master Jun 24, 2014
@arturdev
Copy link
Collaborator

Thanks!

@matthewmoss
Copy link

After this commit, the table doesn't auto scroll when a message is sent or recieved. The improved scroll performance is awesome though

@matthewmoss
Copy link

Found the problem. All that was missing >= vs a >.

@mspensieri
Copy link
Contributor Author

@matthewmoss Are you sure this is related to my changes? I haven't had any problems with scrolling after my commit

EDIT: I did some testing and it looks like this commit is actually the source of the issue : 6c7a977

@matthewmoss
Copy link

Oh, I am very sorry. I noticed the issue after that commit, but was told that this was because of yours. Since that time I found the problem. The first if statement needed to be >= rather than >, but thank you for getting back to me so quickly!

@mspensieri
Copy link
Contributor Author

No worries :) you should open a pull request with that fix, and see if you can get my changes un-reverted !

@matthewmoss
Copy link

Defiantly will, the scroll enhancements are great and it will be a real shame if they are not included.

@arturdev
Copy link
Collaborator

Check #13
Your version has a bug, so I can't merge it yet.

@mspensieri
Copy link
Contributor Author

@arturdev As pointed out above, the bug was actually introduced with this commit : 6c7a977

@matthewmoss seems to have found a fix for it, though

@matthewmoss
Copy link

@mspensieri Ok, issued a pull request. Hope your scrolling improvements will now be included.

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.

4 participants