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

port profile page to backbone.js #5180

Merged
merged 15 commits into from Sep 17, 2014

Conversation

@Raven24
Copy link
Member

commented Aug 29, 2014

this will port the javascript on profile pages
/people/[guid] and
/u/[username]
to use a backbone view.

Not finished, will create a backbone model in order to handle rendering more gracefully,
also, the persons stream doesn't show at this stage.

@Raven24 Raven24 force-pushed the Raven24:profile_status branch from 42b6d8d to 9b3056d Sep 5, 2014

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2014

alright, another huge commit
it would be good if I could already get some feedback on the controller changes

basically the profile sharing indicator badge updates (fetches the profile from the server) either when the user is blocked or when the aspects are changed and then redraws.
now what's still missing is unblocking via backbone (and, of course, some tests)

end
end
end
@aspect = :profile # what does this do?

This comment has been minimized.

Copy link
@jhass

jhass Sep 5, 2014

Member

grep points to https://github.com/diaspora/diaspora/blob/develop/app/helpers/aspect_global_helper.rb#L43

We probably should try to find a way to get rid of that hack, but that's a separate issue.

@contacts_of_contact = @contact.contacts.limit(8)
else
@contact ||= Contact.new
end

This comment has been minimized.

Copy link
@jhass

jhass Sep 5, 2014

Member

I wonder if we shouldn't move that stuff into the presenter or even a distinct presenter.

This comment has been minimized.

Copy link
@Raven24

Raven24 Sep 6, 2014

Author Member

well, it's setting all the @variables for desktop and mobile view, so not sure if a presenter would be much cleaner...

This comment has been minimized.

Copy link
@jhass

jhass Sep 6, 2014

Member

Well, the presenter would be passed to the view and provide methods to access the values we currently assign to these variables.

@person = Person.find_from_guid_or_username(params)

authenticate_user! if remote_profile_with_no_user_session?
raise Diaspora::AccountClosed if @person.closed_account?

This comment has been minimized.

Copy link
@jhass

jhass Sep 5, 2014

Member

Duplicated -> before_action (the find too)

end

def method_missing(method, *args)
@presentable.send(method, *args) if @presentable.respond_to?(method)

This comment has been minimized.

Copy link
@jhass

jhass Sep 5, 2014

Member

public_send and no if so we retain the NoMethodErrors

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2014

thanks for the feedback, will include it in the next iteration.
ok, now all the basic actions on that page should be backbone/ajax. I still have to port some parts of the haml views to handlebars templates. also, tests are still missing...

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2014

another push adding a few parts of what was missing for the templates (+ many dependencies)
... committing/pushing that now since I am getting a bit too tired to produce any more code with measurable quality at this hour ;)

- aspects = stream.aspects
- aspect = stream.aspect
- aspect_ids = stream.aspect_ids

This comment has been minimized.

Copy link
@jhass

jhass Sep 8, 2014

Member

Too much logic in view -> helper.

aspect_ids == current_user.aspect_ids

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Sep 8, 2014

Thanks for your work on that Florian, to have to refresh the page to see the changes after adding someone to an aspect was really annoying.

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2014

alright, I should now have all the "old" functionality working again.
I will include the latest feedback and make a round of cleanup + tests for the next iteration.

... I'm getting there

@jhass

This comment has been minimized.

Copy link
Member

commented Sep 8, 2014

I guess a rebase couldn't hurt to get some travis runs too ;)

@Raven24 Raven24 force-pushed the Raven24:profile_status branch from f381abe to d9d8d5b Sep 8, 2014

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 10, 2014

moar commits...

@@ -0,0 +1,51 @@
app.models.Person = Backbone.Model.extend({
urlRoot: '/people',

This comment has been minimized.

Copy link
@jhass

jhass Sep 10, 2014

Member

Adding js-routes but not using it in new code?

elsif is_sharing then :sharing
elsif is_receiving then :receiving
else :not_sharing
end

This comment has been minimized.

Copy link
@jhass

jhass Sep 10, 2014

Member
return :not_sharing unless contact

[:mutual, :sharing, :receiving].find {|status|
  contact.public_send("#{status}?")
} || :not_sharing
l: url(:scaled_full)
}
}
end

This comment has been minimized.

Copy link
@jhass

jhass Sep 10, 2014

Member

I somehow hate single letter names :)

This comment has been minimized.

Copy link
@Raven24

Raven24 Sep 10, 2014

Author Member

I thought it might be ok here, since w/h and s/m/l are in a context that doesn't leave much room for interpretation.

This comment has been minimized.

Copy link
@jhass

jhass Sep 10, 2014

Member

Sure, not much room for interpretation but it does add another step. When reading the code the brain has to map the abbreviation to the full term in order to understand it.

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 11, 2014

included latest feedback, also rspec should now build green again

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2014

next stop: cukes

Raven24 added 10 commits Aug 29, 2014
* cleanup people_controller#show, add people_controller#stream for json
* introduce new presenters and extend the functionality of the BasePresenter
* add a handlebars template for the profile sidebar, render it everytime we need to update
* introduce a 'aspect_membership:update' global event
* rename profile header view to make name more accurate
* include 'js-routes' for rails routes in javascript
  (TODO: config options?)
* add handlebars helper for rails routes
* refactored text direction detector into helper (also for handlebars)
* added handlebars helper for markdown formatting
* finished port of profile sidebar view to handlebars template
* people_controller refactoring
* add a (hash)tag helper for handlebars
* re-add stream on profile page
* more controller refactoring

@Raven24 Raven24 force-pushed the Raven24:profile_status branch from 55ffa52 to 3004960 Sep 16, 2014

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2014

only three cukes failing anymore...

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 16, 2014

the failing cukes are partly an ugly race condition with ajax vs. capybara finding an element that gets replaced moments later...
might need some more brainpower.

@jhass

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

Let's try

diff --git a/features/step_definitions/user_steps.rb b/features/step_definitions/user_steps.rb
index d410163..efe89a9 100644
--- a/features/step_definitions/user_steps.rb
+++ b/features/step_definitions/user_steps.rb
@@ -124,8 +124,9 @@ When /^I (?:add|remove) the person (?:to|from) my "([^\"]*)" aspect$/ do |aspect
     aspect = find(".dropdown.active .dropdown_list li", text: aspect_name)
     aspect.click
     aspect.parent.has_css?(".loading")
-    aspect.parent.should_not have_css(".loading")
+    aspect.parent.should have_no_css(".loading")
     aspects_dropdown.click
+    aspects_dropdown.should have_no_xpath("..[contains(@class, 'active')]")
 end

 When /^I post a status with the text "([^\"]*)"$/ do |text|
@jhass

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

And for the other failure let's just remove the confusing part:

diff --git a/features/desktop/posts_from_main_page.feature b/features/desktop/posts_from_main_page.feature
index 0e40234..fd829e4 100644
--- a/features/desktop/posts_from_main_page.feature
+++ b/features/desktop/posts_from_main_page.feature
@@ -95,12 +95,8 @@ Feature: posting from the main page

     Scenario: back out of posting a photo-only post
       Given I expand the publisher
-      And I have turned off jQuery effects
       And I attach "spec/fixtures/button.png" to the publisher
-      And I confirm the alert
-      Then I should not see an uploaded image within the photo drop zone
-      And I attach "spec/fixtures/button.png" to the publisher
-      And I click to delete the first uploaded photo
+      When I click to delete the first uploaded photo
       Then I should not see an uploaded image within the photo drop zone
       And I should not be able to submit the publisher
@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2014

alright ... fingers crossed :P

@Flaburgan

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

@Raven24 Raven24 changed the title [WIP] port profile page to backbone.js port profile page to backbone.js Sep 17, 2014

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2014

oops... one litte thing I forgot, then it's ready
writing patch

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2014

done

@Raven24 Raven24 force-pushed the Raven24:profile_status branch from 91c37af to 6e1bd72 Sep 17, 2014

@jhass jhass merged commit 6e1bd72 into diaspora:develop Sep 17, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
jhass added a commit that referenced this pull request Sep 17, 2014
Merge pull request #5180 from Raven24/profile_status
port profile page to backbone.js
@jhass

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

Alright, big enough already. If any remaining issues occur we can have smaller follow ups. Thank you!

@jhass jhass added this to the next-major milestone Sep 17, 2014

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2014

thanks for your continued help with this one ;)

@jhass

This comment has been minimized.

Copy link
Member

commented Sep 17, 2014

Okay, two issues I didn't notice before: The bar under the profile picture seems to be far further away than before and upon following/unfollowing the stream reloads which makes it feel like the entire page reloads which doesn't feel good. Well and I guess I don't like how the publisher sits there on your own profile page either, though I'm not sure what to change (in case we really want to keep it there) :)

@goobertron

This comment has been minimized.

Copy link

commented Sep 18, 2014

Many thanks for this, Florian and Jonne!

@Raven24

This comment has been minimized.

Copy link
Member Author

commented Sep 18, 2014

  1. should be fixed with 926556c
  2. ... well, that's more or less a compromise. I was hoping the stream would just "silently" update, but It hides and displays again after fetching, instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.