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

Buttons for ignore, hide, delete on SPV #5547

Merged
merged 1 commit into from Jan 25, 2015

Conversation

Projects
None yet
4 participants
@Faldrian
Copy link
Contributor

Faldrian commented Jan 11, 2015

This is my attempt to add "delete own post", "ignore user" and "hide this post" buttons to the single post view. This will also move the "report post" button to a semantic better place - away from normal post interaction buttons.

There are some open points where I need further feedback on how to do this right:

  • Position of new buttons
  • If "delete post", "hide post" or "ignore user" action is performed, the browser will load "/stream" on success to lead the user away from the unwanted / deleted post. There may be need for further info messages?

(Also there are no tests yet.)

Screenshot of other users posts:
otehrpost

Screenshot of own post:
ownpost

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 13, 2015

Position of new buttons

That's a good place in my opinion

If "delete post", "hide post" or "ignore user" action is performed, the browser will load "/stream" on success to lead the user away from the unwanted / deleted post. There may be need for further info messages?

We certainly need a confirmation message depending of the action performed. And if you're able to scroll back to the place the stream was before going to the SPV, it would be awesome.

Thanks for this very valuable work!

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 13, 2015

I know we need a message - I just don't know how to implement. ;)

If I change back to the stream, I need to have a way to show a toast-message when the steam is loaded. It seems to me there is no way to tell the "/stream" page that it should show such a message - by simply navigating to "/stream".
There are examples in current code where a message is displayed after navigation to a different url, but this is (as far as I know) always connected to some action being done by the loaded page (instead of doing the action via ajax and then calling the page normally).

So here I need input how to implement / show a confirmation message when an action was done.

And if you're able to scroll back to the place the stream was before going to the SPV, it would be awesome.

That won't be possible, because SPV is not necessarily opened from the stream but may be opened by direct link, in a new tab etc.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 13, 2015

That won't be possible, because SPV is not necessarily opened from the stream but may be opened by direct link, in a new tab etc.

So we need a "if" ;)

About the message, you can check the "You're already logged in": logout and open diaspora in two tabs, then log in in each tabs. You'll have the "You're already logged in" message in the second one (if I remember correctly). So this is an example of a message displayed in the stream from a different page.

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 14, 2015

So we need a "if" ;)

No, we just don't need that feature.

About the message, you can check the "You're already logged in": logout and open diaspora in two tabs, then log in in each tabs. You'll have the "You're already logged in" message in the second one (if I remember correctly). So this is an example of a message displayed in the stream from a different page.

When you log in your browser, you sent a POST request stating what you want to do to the server. So the server can device you already have an open session (depending on the cookie) and display that message when the site is loaded.

In my case that will not work, because the server knows nothing where the user came from or what the user has done when "/stream" will be loaded. All requests and action are done before via ajax and only then the "/stream" is navigated to.
I also don't want to use other methods to block, ignore and delete posts / users, because the existing methods are okay and one single point to maintain.

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 20, 2015

Maybe @svbergerem may have an idea how to make usable notifications when an action (delete own post / hide post / block author) has gone through and the user will be forwarded to "/stream" in the next seconds or so?

Maybe show a toast message with "This post was hidden. You will be forwarded to your stream in 5s." and have the site forward after the timeout?

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Jan 23, 2015

I don't see why we need a message - the user is already asked a confirmation and accepts that. It should be enough to "figure out" what happened and why he/she is now in the stream.

Great work :)

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 23, 2015

Okay... :)
So I will add some tests this weekend and this PR should be good to go?

});

// return to stream
document.location.href = "/stream";

This comment has been minimized.

@jaywink

jaywink Jan 23, 2015

Contributor

Shouldn't this trigger only after ajax comes back as success?

This comment has been minimized.

@Faldrian

Faldrian Jan 23, 2015

Contributor

I implemented this analog to the hidePost in stream_post_view.js - maybe it would be good to change that function, too. Remove / Redirect on success, show an error toast if something went wrong?

This comment has been minimized.

@jaywink

jaywink Jan 23, 2015

Contributor

Yeah that sounds safer to me :)

this.model.blockAuthor()
.done(function() {
// return to stream
document.location.href = "/stream";

This comment has been minimized.

@jaywink

jaywink Jan 23, 2015

Contributor

I guess there is no way to not hardcode links into the JS files... tried searching for existing ones, couldn't seem to figure out if we have anything. It would be nice to push all app paths to gon at some point, but that is probably not in scope of this.

This comment has been minimized.

@jaywink

jaywink Jan 23, 2015

Contributor

On second thought, I guess this might be solved by backbonification of the stream.

This comment has been minimized.

@Faldrian

Faldrian Jan 23, 2015

Contributor

Maybe I could introduce some app.const.locations.stream and set it to "/stream"... but I would do it in another PR for the whole JS-part in one go (if we need that). We need a proper naming scheme for that.

This comment has been minimized.

@jhass

jhass Jan 23, 2015

Member

We actually have js-routes in the Gemfile, someone(tm) should go through the hardcoded routes and update them...

This comment has been minimized.

@Faldrian

Faldrian Jan 23, 2015

Contributor

I add that to my list...

@@ -0,0 +1,47 @@
app.views.SinglePostModeration = app.views.Feedback.extend({

This comment has been minimized.

@jaywink

jaywink Jan 23, 2015

Contributor

Maybe a screenshot of this view in action? :)

This comment has been minimized.

@Faldrian

Faldrian Jan 23, 2015

Contributor

This view is only the little part of the screenshots where the 3 icons (hide, block, report) / 1 icon (delete) is displayed. So: There is already a screenshot. :)

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Jan 23, 2015

Looks good apart from the comments! 🍻

@Faldrian Faldrian force-pushed the Faldrian:spv_moderation_buttons branch from 0f11325 to a7dddae Jan 24, 2015

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 24, 2015

I added tests for all 4 buttons and when the tests go green (seems like some test involving hovercards is failing for several builds, but only in one configuration...?) it could be merged. :)

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Jan 24, 2015

Great work, works very well, tested locally. One more request though, could you please squash the 4 commits to one for a cleaner commit log? Can then do the merge.

Thanks! :)

@jaywink jaywink added this to the next-major milestone Jan 24, 2015

@Faldrian Faldrian force-pushed the Faldrian:spv_moderation_buttons branch from a7dddae to d7cfe71 Jan 25, 2015

@Faldrian Faldrian force-pushed the Faldrian:spv_moderation_buttons branch from d7cfe71 to 31c39a5 Jan 25, 2015

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Jan 25, 2015

🍻 to you, thanks!

jaywink added a commit that referenced this pull request Jan 25, 2015

Merge pull request #5547 from Faldrian/spv_moderation_buttons
Buttons for ignore, hide, delete on SPV

@jaywink jaywink merged commit cc52a5b into diaspora:develop Jan 25, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@Faldrian Faldrian deleted the Faldrian:spv_moderation_buttons branch Jan 25, 2015

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 25, 2015

Hey! Thank you for working on this, this is a really nice feature.
Looks like there is place for an improvement though:

screen

When a post is reshared, the actions can be made on the reshare, not on the original post. So, like the like, comment and reshare icons, the icons added by this pull request should be displayed in the reshare div, not in the original div.

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 25, 2015

Seems logical, thanks for pointing that out. I will add it to my list ... and come back to it when I have time.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 25, 2015

Sure ;) just point if you don't have time in the coming weeks as this is already merged in develop.

@Faldrian

This comment has been minimized.

Copy link
Contributor

Faldrian commented Jan 25, 2015

If you want you can also do this change (and add tests for it) - I'm currently working on another issue.

@Flaburgan

This comment has been minimized.

Copy link
Member

Flaburgan commented Jan 25, 2015

I'm also working on a different issue ^^ but we'll see.

@jaywink

This comment has been minimized.

Copy link
Contributor

jaywink commented Jan 25, 2015

@fla, care to file a bug so we can track that?

On 25.01.2015 22:48, Fla wrote:

I'm also working on a different issue ^^ but we'll see.


Reply to this email directly or view it on GitHub
#5547 (comment).


Br,
Jason Robinson
https://jasonrobinson.me

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