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

Issue #658 patch: updating landing page for screened comments #660

Merged
merged 1 commit into from
Apr 4, 2014

Conversation

fhocutt
Copy link
Contributor

@fhocutt fhocutt commented Mar 28, 2014

Tested on my dreamhack:

Replied to the journal owner's own screened comment: http://test-free.quartzpebble.hack.dreamwidth.net/1202.html?view=1458#cmt1458
Result:
Your comment has been added. According to this journal's settings, it was marked as screened, and will be visible only to you until you choose to unscreen it. View your comment.

Commented here, replying to another journal's screened comment: http://test-free.quartzpebble.hack.dreamwidth.net/1202.html?thread=1202#cmt1202
Result:
Your comment has been added. According to this journal's settings, it was marked as screened, and will be visible only to you until you choose to unscreen it. View your comment.

Replying to own screened comment on own post in community. http://test-comm-free.quartzpebble.hack.dreamwidth.net/443.html?view=699#cmt699
Result:
Your comment has been added. According to this community or entry's settings, it was marked as screened, and will be visible only to you and any other community admins until one of you chooses to unscreen it. View your comment.

Community administrator replies to non-member's screened comment. http://test-comm-free.quartzpebble.hack.dreamwidth.net/443.html?view=1211#cmt1211
Result:
Your comment has been added. According to this community or entry's settings, it was marked as screened, and will be visible only to you and any other community admins until one of you chooses to unscreen it. View your comment.

@afuna
Copy link
Member

afuna commented Mar 31, 2014

Thumbs up on the commit -- that looks good.

I like how much detail you've given re: what you've tested. That gives a lot of additional context, and is very helpful for review. I encourage you to continue doing that!

However, I think that it would be better to have the commit message be a short summary of what changed, and then put all that detail in the pull request.

I'd also encourage you to clean up the commit history, by doing a rebase to merge those two commits into one (since the second one is just a cleanup). I'm happy to catch you online in IRC and talk you through it; fey's also pretty good at explaining this if that works out better for you.

So to recap, what I'd like to happen here is:

  • take the current commit message and paste it as a comment to this pull request (because after you amend the commit message, the text will be gone from here)
  • to squash those two commits into one
  • to have the commit message amended to something like, "Use descriptive link text instead of 'click here' after unscreening"

And then I think we'll be good to go.

@rahaeli rahaeli added this to the Pull Requests milestone Apr 1, 2014
@fhocutt
Copy link
Contributor Author

fhocutt commented Apr 4, 2014

If the new commit message is ok, I think that this is what you've asked for.

afuna added a commit that referenced this pull request Apr 4, 2014
Issue #658 patch: updating landing page for screened comments
@afuna afuna merged commit 45115fb into dreamwidth:develop Apr 4, 2014
@afuna
Copy link
Member

afuna commented Apr 4, 2014

Yes, that is perfect, thank you!

@afuna
Copy link
Member

afuna commented Apr 4, 2014

(and hey congrats on your first commit \o/)

@fhocutt fhocutt deleted the bug658github-screenedlandingpage branch April 4, 2014 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants