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.

Sign up for GitHub

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

Jump to bottom

Mark read/unread displays the notification alert link wrongly (counter) #352

Closed
Eldenroot opened this issue Mar 30, 2024 · 10 comments · Fixed by #361
Closed

Mark read/unread displays the notification alert link wrongly (counter) #352

Eldenroot opened this issue Mar 30, 2024 · 10 comments · Fixed by #361

Comments

@Eldenroot
Copy link
Contributor

image
@lairdshaw
Copy link
Contributor

What are the contents of your myalerts_headericon template?

@Eldenroot
Copy link
Contributor Author

Eldenroot commented Mar 30, 2024 •
edited
Loading 

    • <a href="{$mybb->settings['bburl']}/alerts.php" class="myalerts" onclick="MyBB.popupWindow('/alerts.php?modal=1&amp;ret_link={$myalerts_return_link}', { fadeDuration: 250, zIndex: (typeof modal_zindex !== 'undefined' ? modal_zindex : 9999) }); return false;">{$lang->myalerts_alerts}
    ({$mybb->user['unreadAlerts']})</a>

    @lairdshaw
    Copy link
    Contributor

    lairdshaw commented Mar 31, 2024 •
    edited
    Loading

    OK, that looks correct. Next attempt: Is it fixed if you force-reload the page? I forgot to bump the version of the Javascript file in <head>, so your browser might not be loading the most recent version.

    @Eldenroot
    Copy link
    Contributor Author

    Relaod page - fixes it. However, it happens only when you mark your alert read and unread again.

    @Eldenroot
    Copy link
    Contributor Author

    Btw I use this version with this PR incorporated

    #350

    @lairdshaw
    Copy link
    Contributor

    Relaod page - fixes it. However, it happens only when you mark your alert read and unread again.

    Can you please confirm that after force-refreshing, it's still doing this when marking an alert read and unread again? If so, I'll need to take a look at the board on which this is occurring.

    @lairdshaw
    Copy link
    Contributor

    lairdshaw commented Apr 18, 2024 •
    edited
    Loading

    OK, after checking on your board, the problem seems to be that in your myalerts_headericon template, the ({$mybb->user['unreadAlerts']}) occurs on a new line. This violates the expectation of the Javascript, which is that the opening parenthesis of that term is preceded by a space character. My expectation is that adding that preceding space will resolve this issue for you.

    Alternatively, in jscripts/myalerts.js in the filesystem, find this line:

                    let openParenPos = str.lastIndexOf(' (');

    and remove the space from the ' (' string literal.

    Perhaps the second is the more general fix we should apply to the plugin?

    [Edited to correct the Javascript line to edit and then to style it correctly as Javascript, not PHP]

    @Eldenroot
    Copy link
    Contributor Author

    The second solution is better I think.

    @lairdshaw
    Copy link
    Contributor

    Agreed. I think if we are to implement that fix, then we also should stop prefixing the new alert counts with spaces, so, we'd want to remove the space from the ' (' string literals on these lines too:

                        $hdr_alerts_el.html(MybbStuff.MyAlerts.prototype.stripParenAppendix(hdr_alerts_text) + ' (' + unread_count_fmt + ')');
                        window.document.title = title_bare + ' (' + unread_count_fmt + ')';
                            $('.usercp_nav_myalerts').html('<strong>' + sb_text_bare + ' (' + unread_count_fmt + ')</strong>');

    Can you please test this on your board and make sure it works?

    @Eldenroot
    Copy link
    Contributor Author

    Works fine ,thank you!

    Eldenroot added a commit to Eldenroot/MyAlerts that referenced this issue May 11, 2024
    @Eldenroot
    Fixes MyBBStuff#352
    62b6238
    @Eldenroot Eldenroot mentioned this issue May 11, 2024
    Merged
    lairdshaw pushed a commit that referenced this issue May 11, 2024
    @Eldenroot
    Fixes #352 (#361)
    edafad1
    @lairdshaw lairdshaw mentioned this issue Jun 1, 2024
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    None yet
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging a pull request may close this issue.

    Fixes #352 Eldenroot/MyAlerts
    2 participants
    @Eldenroot @lairdshaw