Bookmark feature #6581

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
6 participants
@Zauberstuhl
Contributor

Zauberstuhl commented Dec 8, 2015

Someone just reminded me of pushing this to the official repo. I am using this on sechat.org
and I am still not happy with it.
I thought some categorization and/or search function should be included in this one.

Suggestions? Objections?

untitled2
untitled

Zauberstuhl added some commits Oct 5, 2015

Initial diaspora favorite/pinboard feature
You can now add or pin favorite posts to your
personal pin-board "/favors".
This is an first approach and not suitable for production

Signed-off-by: Lukas Matt <lukas@zauberstuhl.de>
Rename Favorites to Bookmarks
Signed-off-by: Lukas Matt <lukas@zauberstuhl.de>
Using different controller routines
Signed-off-by: Lukas Matt <lukas@zauberstuhl.de>
Fix flashMessage function in backbone view
Signed-off-by: Lukas Matt <lukas@zauberstuhl.de>
@@ -0,0 +1,4 @@
+app.models.Bookmark = Backbone.Model.extend({
+ baseURL: "/posts/",
+ url: function() { return this.baseURL + '/' + this.id + '/bookmarks'; }

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

@@ -91,6 +94,26 @@ app.views.Base = Backbone.View.extend({
this.model.set(_.inject(this.formAttrs, _.bind(setValueFromField, this), {}));
},
+ bookmark: function(evt) {
+ if(evt) { evt.preventDefault(); }
+ var type = $(evt.currentTarget).data('type');

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

+ var respond = {
+ type: method,
+ success: function() {
+ app.flashMessages.success(Diaspora.I18n.t('bookmark.status.' + type + '.success'));

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

+ app.flashMessages.success(Diaspora.I18n.t('bookmark.status.' + type + '.success'));
+ },
+ error: function() {
+ app.flashMessages.error(Diaspora.I18n.t('bookmark.status.' + type + '.error'));

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

+ };
+
+ var bookmark = new app.models.Bookmark({id: this.model.id});
+ if (method === 'POST') {

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Strings must use doublequote.

+ var bookmark = new app.models.Bookmark({id: this.model.id});
+ if (method === 'POST') {
+ bookmark.save({}, respond);
+ } else bookmark.destroy(respond);

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Expected '{' and instead saw 'bookmark'.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Expected '{' and instead saw 'bookmark'.

+ validates :user_id, presence: true
+ validates :post_id, presence: true
+
+ validate :entry_does_not_exist, :on => :create

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Use the new Ruby 1.9 hash syntax.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Use the new Ruby 1.9 hash syntax.

+ end
+
+ def entry_does_not_exist
+ if item.exists?

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Use a guard clause instead of wrapping the code inside a conditional expression.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Use a guard clause instead of wrapping the code inside a conditional expression.

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

+
+ def entry_does_not_exist
+ if item.exists?
+ errors[:base] << 'You cannot add the same post twice.'

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

+ t.integer :user_id, null: false
+ t.integer :post_id, null: false
+ end
+ add_index :favorites, [:user_id, :post_id], unique: true

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Use %i or %I for array of symbols.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Use %i or %I for array of symbols.

@@ -9,4 +9,5 @@ module Stream
require 'stream/person'
require 'stream/public'
require 'stream/tag'
+ require 'stream/bookmarked'

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@@ -0,0 +1,26 @@
+class Stream::Bookmarked < Stream::Base

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Use nested module/class definitions instead of compact style.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Use nested module/class definitions instead of compact style.

+ end
+
+ def stream_posts
+ self.posts

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Redundant self detected.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Redundant self detected.

+ end
+
+ def contacts_title
+ I18n.translate('streams.bookmarks.contacts_title')

This comment has been minimized.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@diaspora-code-review

diaspora-code-review Dec 8, 2015

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@lu-x

This comment has been minimized.

Show comment
Hide comment
@lu-x

lu-x Dec 9, 2015

My proposal would be to remove the publisher (and maybe add a search later at this point). Also, fading out bookmarked posts after clicking "Remove Boookmark" would be good.
Furthermore the small pop-ups (which we use for the other buttons) for "Add Bookmark" and "Remove Bookmark" are missing.

lu-x commented Dec 9, 2015

My proposal would be to remove the publisher (and maybe add a search later at this point). Also, fading out bookmarked posts after clicking "Remove Boookmark" would be good.
Furthermore the small pop-ups (which we use for the other buttons) for "Add Bookmark" and "Remove Bookmark" are missing.

@noplanman

This comment has been minimized.

Show comment
Hide comment
@noplanman

noplanman Feb 5, 2016

Contributor

I really like the idea of having bookmarks. I was thinking of extending my userscript to include bookmarks, but having them in diaspora* itself would be amazing.

The way I would use bookmarks would be more like a "read later" function:
After reading, if I like it, I "Like" it and it would be saved in the "Liked" list, where I could find it again. Then I remove the bookmark.

@Zauberstuhl
If I understood you correctly, you're talking about a more permanent bookmarking here, right?

Contributor

noplanman commented Feb 5, 2016

I really like the idea of having bookmarks. I was thinking of extending my userscript to include bookmarks, but having them in diaspora* itself would be amazing.

The way I would use bookmarks would be more like a "read later" function:
After reading, if I like it, I "Like" it and it would be saved in the "Liked" list, where I could find it again. Then I remove the bookmark.

@Zauberstuhl
If I understood you correctly, you're talking about a more permanent bookmarking here, right?

@Zauberstuhl

This comment has been minimized.

Show comment
Hide comment
@Zauberstuhl

Zauberstuhl Feb 6, 2016

Contributor

where I could find it again

this is something I tried to achieve ... its not perfect but it works for now... In my opinion finding stuff is one of the biggest problems in d*

permanent bookmarking

well you can remove a bookmark..


In the end .. I haven't finished this PR cause right now I don't like it. It is just 'another' stream..

Contributor

Zauberstuhl commented Feb 6, 2016

where I could find it again

this is something I tried to achieve ... its not perfect but it works for now... In my opinion finding stuff is one of the biggest problems in d*

permanent bookmarking

well you can remove a bookmark..


In the end .. I haven't finished this PR cause right now I don't like it. It is just 'another' stream..

@denschub denschub added the orphan label May 6, 2016

@vanitasvitae

This comment has been minimized.

Show comment
Hide comment
@vanitasvitae

vanitasvitae Oct 9, 2016

So this is not gonna happen? I'd quite like to have a bookmark feature on d* that allows saving my favorite posts synced on multiple devices.

So this is not gonna happen? I'd quite like to have a bookmark feature on d* that allows saving my favorite posts synced on multiple devices.

@noplanman

This comment has been minimized.

Show comment
Hide comment
@noplanman

noplanman Dec 16, 2016

Contributor

@Zauberstuhl Hm, that's unfortunate 😕
Are you going to keep it on sechat.org though?

Contributor

noplanman commented Dec 16, 2016

@Zauberstuhl Hm, that's unfortunate 😕
Are you going to keep it on sechat.org though?

@Zauberstuhl

This comment has been minimized.

Show comment
Hide comment
@Zauberstuhl

Zauberstuhl Dec 16, 2016

Contributor

its long broken on sechat.org

Contributor

Zauberstuhl commented Dec 16, 2016

its long broken on sechat.org

@noplanman

This comment has been minimized.

Show comment
Hide comment
@noplanman

noplanman Dec 16, 2016

Contributor

Oh, I only have a testing account on sechat, so I hadn't noticed...
I feel this is a much wanted feature, maybe it's just not the time yet.

Contributor

noplanman commented Dec 16, 2016

Oh, I only have a testing account on sechat, so I hadn't noticed...
I feel this is a much wanted feature, maybe it's just not the time yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment