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

Conversations: fix badge count and automatic scrolling #5646

Merged
merged 1 commit into from Feb 11, 2015

Conversation

@svbergerem
Copy link
Member

commented Feb 10, 2015

Fix for #4572, #4818 and #3998. I still need to add some tests but the code I wrote so far should be ready to review. Especially the controller changes might need some improvement. (was more or less trial and error)

cBadge = $("#conversations_badge .badge_count");

if(conv.hasClass('unread') ){
var unreadCount = parseInt(conv.find('.unread_message_count').text());

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2015

Missing radix parameter.


if(cBadge.text() !== '') {
cBadge.text().replace(/\d+/, function(num){
num = parseInt(num) - unreadCount;

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 10, 2015

Missing radix parameter.

@visibility.unread = 0
@visibility.save
end
end

This comment has been minimized.

Copy link
@jhass

jhass Feb 10, 2015

Member

Might as well move this method to the Conversation model.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 10, 2015

Author Member

done

@@ -17,7 +17,7 @@
#conversation_inbox
.stream.conversations
- if @conversations.count > 0
= render :partial => 'conversations/conversation', :collection => @conversations, :locals => {:authors => @authors, :ordered_participants => @ordered_participants, :unread_counts => @unread_counts}
= render :partial => 'conversations/conversation', :collection => @conversations, :locals => {:authors => @authors, :ordered_participants => @ordered_participants, :unread_counts => @unread_counts, :selected_conversation_id => (@conversation ? @conversation.id : -1)}

This comment has been minimized.

Copy link
@jhass

jhass Feb 10, 2015

Member

@conversation.try(:id) || -1, but I'm not sure how I feel about using -1 as a magic value here, why not check for nil in the target code?

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 10, 2015

Author Member

done

@@ -3,7 +3,9 @@
-# the COPYRIGHT file.
.conversation-wrapper{ :"data-conversation-path" => conversation_path(conversation) }
.stream_element.conversation{:data=>{:guid=>conversation.id}, :class => ('unread' if unread_counts[conversation.id].to_i > 0)}
- conversation_class = (unread_counts[conversation.id].to_i > 0 ? 'unread' : '')
- conversation_class << (conversation.id == selected_conversation_id ? ' selected' : '')

This comment has been minimized.

Copy link
@jhass

jhass Feb 10, 2015

Member

Should probably be a helper method.

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 10, 2015

Author Member

done

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

Hm, it's probably not related to what you changed here but did you have a look at #5330 in the same time? This is a regression which is not in the current master, so it would be nice to see it fixed.

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2015

@Flaburgan No. As you said this is not related and in this PR I just want to fix the issues I already mentioned.

@svbergerem svbergerem force-pushed the svbergerem:fix-conversation-scrolling branch from d4ebd16 to 8962d75 Feb 10, 2015

@svbergerem svbergerem changed the title [WIP] Conversations: fix badge count and automatic scrolling Conversations: fix badge count and automatic scrolling Feb 10, 2015

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 10, 2015

All tests succeeded. Should be ready to review and merge.

@jhass jhass added this to the next-major milestone Feb 11, 2015

@jhass jhass merged commit 8962d75 into diaspora:develop Feb 11, 2015

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
hound Hound has reviewed the changes.

jhass added a commit that referenced this pull request Feb 11, 2015

Merge pull request #5646 from svbergerem/fix-conversation-scrolling
Conversations: fix badge count and automatic scrolling
@jhass

This comment has been minimized.

Copy link
Member

commented Feb 11, 2015

Looks solid, thank you!

@svbergerem svbergerem deleted the svbergerem:fix-conversation-scrolling branch Feb 11, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Feb 12, 2015

Looks like we now have a problem trying to access the conversation page:

Processing by ConversationsController#index as HTML
  Rendered conversations/_conversation.haml (89.6ms)
  Rendered conversations/index.haml within layouts/with_header (92.1ms)
Completed 500 Internal Server Error in 353ms

ActionView::Template::Error (undefined method `conversation_class' for #<#:0x0000326afc3d10>):
    3: -#   the COPYRIGHT file.
    4: 
    5: .conversation-wrapper{ :"data-conversation-path" => conversation_path(conversation) }
    6:   .stream_element.conversation{:data=>{:guid=>conversation.id}, :class => conversation_class(conversation, unread_counts[conversation.id].to_i, selected_conversatio
n_id)}
    7:     .media
    8:       .img
    9:         - other_participants = ordered_participants[conversation.id] - [current_user.person]
  app/views/conversations/_conversation.haml:6:in`_app_views_conversations__conversation_haml__704969970413809694_108277623820'
  app/views/conversations/index.haml:20:in `_app_views_conversations_index_haml___1310477773073387960_108271293020'
  app/controllers/conversations_controller.rb:32:in`index'
  lib/rack/chrome_frame.rb:39:in `call'
  lib/unicorn_killer.rb:35:in`call'
@marienfressinaud

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2015

@Flaburgan +1, but no problem on mobile

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Feb 12, 2015

Looks like something went wrong during the assets compilation, I cleaned them and re precompile them and now everything is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.