Collapsing changes #11

Merged
merged 2 commits into from Mar 15, 2013

Conversation

Projects
None yet
3 participants
@stevenleeg
Contributor

stevenleeg commented Mar 12, 2013

I've been loving HackerNew since I installed it, but one thing I missed was reddit-like comment collapsing from a plugin I was previously using called Hacker News Collapse.

This pull request implements HN Collapse in HackerNew. It's quite a bit less showy than the current implementation, but I think it does a good job of making comment collapsing fast and easily accessible.

Here's what it looks like before:
Before collapse

And after:
After collapse

Let me know what you think!

@tommoor

This comment has been minimized.

Show comment Hide comment
@tommoor

tommoor Mar 12, 2013

Owner

Hey Steven,

It's probably better to have the collapse functionality over on the left, it does better associate it with the content that you're collapsing. Does this patch do anything other than change the styling, or is it just a visual tweak?

Owner

tommoor commented Mar 12, 2013

Hey Steven,

It's probably better to have the collapse functionality over on the left, it does better associate it with the content that you're collapsing. Does this patch do anything other than change the styling, or is it just a visual tweak?

@stevenleeg

This comment has been minimized.

Show comment Hide comment
@stevenleeg

stevenleeg Mar 12, 2013

Contributor

Ah to each his own. It's mostly a visual tweak (check out the diff), so if you don't want to include it feel free to close this.

Keep up the awesome job on HackerNew!

On Tue, Mar 12, 2013 at 1:30 AM, Tom Moor notifications@github.com
wrote:

Hey Steven,

It's probably better to have the collapse functionality over on the left, it does better associate it with the content that you're collapsing. Does this patch do anything other than change the styling, or is it just a visual tweak?

Reply to this email directly or view it on GitHub:
#11 (comment)

Contributor

stevenleeg commented Mar 12, 2013

Ah to each his own. It's mostly a visual tweak (check out the diff), so if you don't want to include it feel free to close this.

Keep up the awesome job on HackerNew!

On Tue, Mar 12, 2013 at 1:30 AM, Tom Moor notifications@github.com
wrote:

Hey Steven,

It's probably better to have the collapse functionality over on the left, it does better associate it with the content that you're collapsing. Does this patch do anything other than change the styling, or is it just a visual tweak?

Reply to this email directly or view it on GitHub:
#11 (comment)

@christiangenco

This comment has been minimized.

Show comment Hide comment
@christiangenco

christiangenco Mar 13, 2013

+1 on having it on the left side. This is how the Reddit Enhancement suite does things:

Screen Shot 2013-03-13 at 1 16 57 PM

Which is super nice, because to collapse a whole bunch of comments in series you only have to move your mouse horizontally. From a design standpoint, that makes a lot more sense.

+1 on having it on the left side. This is how the Reddit Enhancement suite does things:

Screen Shot 2013-03-13 at 1 16 57 PM

Which is super nice, because to collapse a whole bunch of comments in series you only have to move your mouse horizontally. From a design standpoint, that makes a lot more sense.

@tommoor

This comment has been minimized.

Show comment Hide comment
@tommoor

tommoor Mar 13, 2013

Owner

Ah interesting, I definitely feel this is a better direction than the current implementation - perhaps I could pull this and tweak it further so that the collapse is on the left.

Owner

tommoor commented Mar 13, 2013

Ah interesting, I definitely feel this is a better direction than the current implementation - perhaps I could pull this and tweak it further so that the collapse is on the left.

@christiangenco

This comment has been minimized.

Show comment Hide comment
@christiangenco

christiangenco Mar 13, 2013

.prepend instead?

.prepend instead?

This comment has been minimized.

Show comment Hide comment
@stevenleeg

stevenleeg Mar 14, 2013

Owner

Got it! I fixed it in 2efb623.

Owner

stevenleeg replied Mar 14, 2013

Got it! I fixed it in 2efb623.

This comment has been minimized.

Show comment Hide comment
@christiangenco

christiangenco Mar 15, 2013

Woohoo!

Woohoo!

@christiangenco

This comment has been minimized.

Show comment Hide comment
@christiangenco

christiangenco Mar 13, 2013

No need to hide/show on hover if it's just [-] imo. Not that much saved space.

No need to hide/show on hover if it's just [-] imo. Not that much saved space.

This comment has been minimized.

Show comment Hide comment
@stevenleeg

stevenleeg Mar 14, 2013

Owner

What do you mean? This isn't doing anything on hover as far as I can see.

Owner

stevenleeg replied Mar 14, 2013

What do you mean? This isn't doing anything on hover as far as I can see.

This comment has been minimized.

Show comment Hide comment
@christiangenco

christiangenco Mar 15, 2013

Ahh nevermind - I misread this.

Ahh nevermind - I misread this.

@stevenleeg

This comment has been minimized.

Show comment Hide comment
@stevenleeg

stevenleeg Mar 13, 2013

Contributor

I'm stuck in an airport for a bit so I can tweak it and see if you'd like it. Hang tight and I'll get something pushed!

On Wed, Mar 13, 2013 at 2:19 PM, Tom Moor notifications@github.com
wrote:

Ah interesting, I definitely feel this is a better direction than the current implementation - perhaps I could pull this and tweak it further so that the collapse is on the left.

Reply to this email directly or view it on GitHub:
#11 (comment)

Contributor

stevenleeg commented Mar 13, 2013

I'm stuck in an airport for a bit so I can tweak it and see if you'd like it. Hang tight and I'll get something pushed!

On Wed, Mar 13, 2013 at 2:19 PM, Tom Moor notifications@github.com
wrote:

Ah interesting, I definitely feel this is a better direction than the current implementation - perhaps I could pull this and tweak it further so that the collapse is on the left.

Reply to this email directly or view it on GitHub:
#11 (comment)

@stevenleeg

This comment has been minimized.

Show comment Hide comment
@stevenleeg

stevenleeg Mar 13, 2013

Contributor

There we are:

Screenshot

Screenshot

Contributor

stevenleeg commented Mar 13, 2013

There we are:

Screenshot

Screenshot

@tommoor

This comment has been minimized.

Show comment Hide comment
@tommoor

tommoor Mar 13, 2013

Owner

Looking good! How are you finding it functionally? I'll be able to check it out a bit later on... two things:

  • Are the retina arrows not working for you? They look kind of crappy...
  • I wonder if we can size the + and - to be the same size - perhaps use an emdash or something longer..
Owner

tommoor commented Mar 13, 2013

Looking good! How are you finding it functionally? I'll be able to check it out a bit later on... two things:

  • Are the retina arrows not working for you? They look kind of crappy...
  • I wonder if we can size the + and - to be the same size - perhaps use an emdash or something longer..
@stevenleeg

This comment has been minimized.

Show comment Hide comment
@stevenleeg

stevenleeg Mar 13, 2013

Contributor

Glad you like it! I've been using my fork for a couple days now and haven't had any problems with the collapsing.

And yeah I've been having trouble with the retina arrows actually, not sure why. It might be some change I made so I'll look into it when I get home later.

On Wed, Mar 13, 2013 at 3:21 PM, Tom Moor notifications@github.com
wrote:

Looking good! How are you finding it functionally? I'll be able to check it out a bit later on... two things:

  • Are the retina arrows not working for you? They look kind of crappy...

- I wonder if we can size the + and - to be the same size - perhaps use an emdash or something longer..

Reply to this email directly or view it on GitHub:
#11 (comment)

Contributor

stevenleeg commented Mar 13, 2013

Glad you like it! I've been using my fork for a couple days now and haven't had any problems with the collapsing.

And yeah I've been having trouble with the retina arrows actually, not sure why. It might be some change I made so I'll look into it when I get home later.

On Wed, Mar 13, 2013 at 3:21 PM, Tom Moor notifications@github.com
wrote:

Looking good! How are you finding it functionally? I'll be able to check it out a bit later on... two things:

  • Are the retina arrows not working for you? They look kind of crappy...

- I wonder if we can size the + and - to be the same size - perhaps use an emdash or something longer..

Reply to this email directly or view it on GitHub:
#11 (comment)

tommoor added a commit that referenced this pull request Mar 15, 2013

@tommoor tommoor merged commit 65364b1 into tommoor:master Mar 15, 2013

@tommoor

This comment has been minimized.

Show comment Hide comment
@tommoor

tommoor Mar 15, 2013

Owner

Merging this as it's definitely an improvment. It looks like loss of retina was related to the HN update to https the other day and is a separate issue.

Owner

tommoor commented Mar 15, 2013

Merging this as it's definitely an improvment. It looks like loss of retina was related to the HN update to https the other day and is a separate issue.

@christiangenco

This comment has been minimized.

Show comment Hide comment
@christiangenco

christiangenco Mar 15, 2013

Nice work @stevenleeg!

Nice work @stevenleeg!

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