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

More consistent relative date display #758

Merged
merged 4 commits into from
May 25, 2023
Merged

Conversation

eritbh
Copy link
Member

@eritbh eritbh commented May 24, 2023

Timeago doesn't support operating on elements before they're in the DOM because it keeps intervals open to update the element's text and doing that for elements that aren't on the page anymore would leak memory. To work around this, we queue timeago to run after a tick so it happens after the rest of the HTML is on the page.

Interestingly I found that Promise.resolve().then(...) works faster than setTimeout(..., 0) in Firefox here - the latter results in significant flashing of non-relative date text when opening the popup, whereas the former has an opportunity to act after the target element is added to the DOM but before the next frame is rendered.

Screen recordings of both versions

Here's the setTimeout(..., 0) version:

6JaDBQy9mEH11i.mp4

And here's Promise.resolve().then(...):

JAPhgvbpyad11g.mp4

@eritbh eritbh added bug something isn't working module: modnotes labels May 24, 2023
@eritbh eritbh added this to the v6.1.5/6.2.0 milestone May 24, 2023
@eritbh
Copy link
Member Author

eritbh commented May 24, 2023

A friend suggested a slightly less hacky way to do this might be with mutation observers. It's pretty much equivalent to what's going on here, doesn't have the flashing issue, and would work even if we didn't add the popup to the page immediately after creating it. The code is a bit more complicated, though, and it seems unlikely to address problems we'd run into since this is very module-local UI stuff rather than anything more general. Thought I'd drop it here anyway for reference.

new MutationObserver((mutations, observer) => {
    mutations.forEach(mutation => {
        if (!mutation.addedNodes.includes($noteRow[0])) {
            return;
        }
        $noteRow.find('time').timeago();
        observer.disconnect();
    });
}).observe(document.body, {subtree: true, childList: true});

@creesch
Copy link
Member

creesch commented May 25, 2023

LGM.

However, isn't this then also an issue in other places where we generate UI before showing it?

@eritbh
Copy link
Member Author

eritbh commented May 25, 2023

From what I can see most of the existing timeago calls we make aren't really scoped the way I'm doing it here. A lot of the time we just $('time.timeago').timeago() which can only apply to things already in the DOM (since $() doesn't know about elements that only exist on the stack). I checked through all the other instances where we call .timeago() and they also all happen after the relevant elements are added to the DOM - except for one, which handles timestamp display in usernotes (and it looks like it was probably broken by me when I introduced TBui.pager() and restructured how notes are rendered there, so it's having the same problem as modnotes).

I'll add a commit to fix the issue there as well, but everything else should be working fine from what I can tell.

@eritbh eritbh changed the title More consistent modnote relative date display More consistent relative date display May 25, 2023
@eritbh
Copy link
Member Author

eritbh commented May 25, 2023

Existing implementation fixed in the usernotes manager, and relative date handling has also been added to the main user notes popup as discussed on Discord.

@eritbh eritbh added enhancement New feature or request module: usernotes labels May 25, 2023
@eritbh eritbh merged commit 4cc6e39 into master May 25, 2023
@eritbh eritbh deleted the fix/modnotes-relative-dates branch May 25, 2023 19:07
@eritbh eritbh mentioned this pull request Jul 4, 2023
eritbh added a commit that referenced this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something isn't working enhancement New feature or request module: modnotes module: usernotes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants