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

Add year to notifications page #5676

Merged
merged 1 commit into from Feb 16, 2015

Conversation

@svbergerem
Copy link
Member

commented Feb 16, 2015

Fixes #5361 and #5673.



def display_year(year, date)
if year.nil?
not Date.current().year === the_year(date)

This comment has been minimized.

Copy link
@jhass

jhass Feb 16, 2015

Member

Date.current.year != the_year(date)

if year.nil?
not Date.current().year === the_year(date)
else
not year === the_year(date)

This comment has been minimized.

Copy link
@jhass

jhass Feb 16, 2015

Member

See above.


def display_year(year, date)

This comment has been minimized.

Copy link
@jhass

jhass Feb 16, 2015

Member

display_year?


def display_year(year, date)
if year.nil?

This comment has been minimized.

Copy link
@jhass

jhass Feb 16, 2015

Member

unless year

This comment has been minimized.

Copy link
@svbergerem

svbergerem Feb 16, 2015

Author Member

Oh, i didn't know that unless also works with else. I will change that.

This comment has been minimized.

Copy link
@jhass

jhass Feb 16, 2015

Member

It does, there's just no elsunless and it doesn't work with elsif.


describe '#display_year' do
it 'returns false if year is nil and the date includes the current year' do
expect(display_year(nil,Date.current().strftime('%Y-%m-%d'))).to be false

This comment has been minimized.

Copy link
@jhass

jhass Feb 16, 2015

Member

No empty parens, be_false or better be_falsey

end

it 'returns true if year is nil and the date does not include the current year' do
expect(display_year(nil,'1900-12-31')).to be true

This comment has been minimized.

Copy link
@jhass

jhass Feb 16, 2015

Member

be_true/``be_truthy`

Steffen van Bergerem

@svbergerem svbergerem force-pushed the svbergerem:notifications-add-year branch from 12f25ca to 9c22ed6 Feb 16, 2015

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

@jhass Thank you for comments. All of those should be fixed now.

@jhass jhass added this to the next-major milestone Feb 16, 2015

@goobertron

This comment has been minimized.

Copy link

commented Feb 16, 2015

Purely from a point of view of personal taste, I'd suggest less space in between the two years in the first screenshot, and possibly the year aligned on the left with the dates rather than centred over the notifications.

To make it more visible, the year could be in the dark red of unread notifications; it could even be placed to the right of the date to save vertical space. Something like this:

_notif2
The red might be a bit over-the-top; grey might be better.

Just some thoughts: I don't dislike what you've done, but I think my suggestions might help to make it a bit neater.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 16, 2015

Thanks!

@jhass jhass merged commit 9c22ed6 into diaspora:develop Feb 16, 2015

2 checks passed

continuous-integration/travis-ci The Travis CI build passed
Details
hound Hound has reviewed the changes.

jhass added a commit that referenced this pull request Feb 16, 2015

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 16, 2015

Ups, looks like I was a bit quick :P

@svbergerem svbergerem deleted the svbergerem:notifications-add-year branch Feb 16, 2015

@goobertron

This comment has been minimized.

Copy link

commented Feb 16, 2015

Or I was too slow!

I was out for a few hours, so only just came back to the computer.

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

@goobertron I think adding the year in red looks too distracting. What I tried to do with the big margin and the centered year was getting the users attention while going through his notifications without distracting him and to keep the focus on the content.

@goobertron

This comment has been minimized.

Copy link

commented Feb 16, 2015

Sure, I understand that, and I'm also not convinced by the red. I'm only thinking of how the design might be tweaked to keep the attention-grabbing aspect without so much white space.

How about two solid lines, with the year in between them - either with the year centred over the whole div or aligned with the dates? Like one of these:

_notif3

_notif3a
(For some reason the upper of the two solid lines doesn't display at the magnification shown on the page on my screen, but zoom in and you'll see it.)

The year (and space between the rules) could even be a bit larger to grab attention more.

I hope you don't mind me suggestion changes, especially as your PR has already been merged!

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

I hope you don't mind me suggestion changes, especially as your PR has already been merged!

Definitely not. Your opinion is always much appreciated. :) The PR has been merged but that doesn't mean that we can't improve it in further PRs.

Those screenshots look much better but I'm still not really convinced. I think in both screenshots there should be some margin to the top. (And then we are basically back to the solution I implemented in this PR) Maybe we need someone else's opinion here.

@jhass

This comment has been minimized.

Copy link
Member

commented Feb 16, 2015

I like the version that I merged. If you want to make it even more obvious I'd make the year at least as big as the day in month, if not bigger. And maybe vertically center it, that is add the same amount of whitespace below and above, not necessarily increasing the total amount of whitespace though.

@goobertron

This comment has been minimized.

Copy link

commented Feb 16, 2015

I should say that I don't object to the amount of white space in the version merged, and given that it will only appear between two years, it won't be an intrusive feature in most people's notification views (it would normally appear only every few pages). But if we can make the year numerals more obvious with less blank vertical space, it would look a bit nicer, I think. What I suggested above (and which Jonne confirmed) could work, like this:

_notif4

Anyway, I'm signing off for tonight - see you both tomorrow. Thanks, both, for your continued excellent work for Diaspora.

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

Ok, now I get what you are talking about. :-P In that screenshot I guess there isn't really less white space but it doesn't feel like unused white space anymore. There is one problem, though. We will have to remove the border of the last notification before the year (you already did that in the screenshot) but that is not that easy. Given you have the following code

.day_group
.day_group
.year_container
.day_group
.day_group

You can define the style of the first .day_group after year_container with

.year_container + .day_group { ... }

but as far as I know there is now way to define the style of the last .day_group before .year_container.

Does anyone have a solution that doesn't need a big refactoring?

@svbergerem

This comment has been minimized.

Copy link
Member Author

commented Feb 16, 2015

Forget it, I solved the problem. :-P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.