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

Ticket/11921 - Streamline PMs and Notifications in Header Link List #1781

Merged
merged 6 commits into from
Oct 16, 2013

Conversation

iMattPro
Copy link
Member

Based on Area51 topic discussion beginning here: http://area51.phpbb.com/phpBB/viewtopic.php?f=84&t=42829&start=260#p256779

@@ -5233,7 +5233,7 @@ function page_header($page_title = '', $display_online_list = true, $item_id = 0
{
if ($user->data['user_new_privmsg'])
{
$l_privmsgs_text = $user->lang('NEW_PMS', (int) $user->data['user_new_privmsg']);
$l_privmsgs_count = (int) $user->data['user_new_privmsg'];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l_ means its a language string, which does not apply for an integer

@iMattPro
Copy link
Member Author

By the way I noticed, there is a boolean flag in this PM logic in functions.php s_privmsg_new that is only used to set a template var called called S_NEW_PM Neither this flag or its template var appear anywhere in use as far as I can tell. I could remove it from the logic in this PR if it is indeed unused. ?

@@ -5350,8 +5348,7 @@ function page_header($page_title = '', $display_online_list = true, $item_id = 0
'TOTAL_USERS_ONLINE' => $l_online_users,
'LOGGED_IN_USER_LIST' => $online_userlist,
'RECORD_USERS' => $l_online_record,
'PRIVATE_MESSAGE_INFO' => $l_privmsgs_text,
'PRIVATE_MESSAGE_INFO_UNREAD' => $l_privmsgs_text_unread,
'PRIVATE_MESSAGE_COUNT' => $privmsgs_count,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm seeing this correct, the behavior you have here is that it shows the count of the new private messages unless there are none, in which case it shows the count of the unread messages.

If so, I think that will be a little confusing, rather I think it should only ever show the count of unread messages (unless there is some way to tell when the count means it's for new messages rather than unread).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take another look, but yeah.. It should always show Unread... New messages are unread messages too, after all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually looks fine. What it does is if new = unread, it shows new... If new != unread, it shows unread.

"new" vanishes the moment you hit the PM page (and appear to only exist for the purpose of PM popup alerts). But "Unread" status stays intact until you actually open the PM.

So the only time it shows "new" is when its the same number as "unread".

So it is basically always showing us what is Unread in our inboxes.

Edit: I have been testing it and it behaves as you would expect - always showing you the count of PMs that are unread.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "(x new, y unread)"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please just set this to (int) $user->data['user_unread_privmsg'] then.

@EXreaction
Copy link
Contributor

Others may want to use the S_NEW_PM variable (in another style or in an extension). I think we should just keep it as is.

@gooof
Copy link

gooof commented Oct 16, 2013

No mod using S_NEW_PM, its only using is in overall_header.html for the PM Popup. ;)

@iMattPro
Copy link
Member Author

Question: Would brackets look better around the Notification and PM count numbers instead of parenthesis? Brackets would be more consistent with the Logout [ username ] used in the link list.

@gooof
Copy link

gooof commented Oct 16, 2013

The best for me is more this way:

- 'NOTIFICATIONS_COUNT'    => 'Notifications (<strong>%d</strong>)',
+ 'NOTIFICATIONS_COUNT'    => 'Notifications <span class="notification-count">%d</span>',

than you can add brackets with css, another color etc.

@nickvergessen
Copy link
Contributor

Do not put HTML into the language, unless required.

EXreaction added a commit that referenced this pull request Oct 16, 2013
Ticket/11921 - Streamline PMs and Notifications in Header Link List
@EXreaction EXreaction merged commit 852b707 into phpbb:develop Oct 16, 2013
@iMattPro iMattPro deleted the ticket/11921 branch October 30, 2013 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants