fregante / notifications-preview-github

Browser Extension: preview GitHub notifications with same page pop-overs
https://chrome.google.com/webstore/detail/github-notifications-prev/kgilejfahkjidpaclkepbdoeioeohfmj?hl=en&gl=IN
MIT License
144 stars 15 forks source link

No longer staying on page after marking as read #70

Closed glitsj16 closed 5 years ago

glitsj16 commented 5 years ago

OS: Arch Linux Browser: Firefox 66.0.1 Notifications Preview for GitHub: 1.5.3

Browser extension - Preview your notifications without leaving the page

This wonderful time-saver extension seems partly broken (due to recent GitHub changes?). I love previewing notifications without leaving the page, but recently I get redirected to https://github.com/notifications after marking stuff read. Is this something that can be fixed somehow?

tanmayrajani commented 5 years ago

I'm guessing we can't return false on submission here since the action will only be performed if it's submitted.

What do you guys think? @bfred-it @jdreesen

Edit: Not sure if it's a good idea but one option is to convert the form with action to async operation like adding a function onsubmit (which performs that action using fetch maybe?) and removing the attributes that let the redirection happen. Thoughts?

fregante commented 5 years ago

We're copying GitHub's own Notifications page, so if it works on that page, it should also work for us. They probably changed some selectors that enabled the functionality and our form doesn't have them (maybe because the selector includes body.notifications or something like that, which we don't have)

fregante commented 5 years ago

But it seems that GitHub no longer loads the notification scripts on the page, so there's no handler at all for the current selector (which should work). From GitHub:

            o(".js-mark-notification-as-unread", async function(i, e) {
                c();
                const s = a(i, ".js-notification");
                n(s),
                f(s, ".js-mark-notification-as-read button");
                try {
                    await e.html()
                } catch (o) {
                    t(s),
                    r()
                }
            }),

I think it can be replaced with form.onsubmit => event.preventDefault()

tanmayrajani commented 5 years ago

So, I did the following:

for (const group of this.list) {
    ...
    for (const form of select.all('form', group)) {
        form.onsubmit = e => e.preventDefault()
    }
}

at https://github.com/tanmayrajani/notifications-preview-github/blob/e4304837764e06f7e7095f47df2a151d68564c6c/extension/index.js#L22:

And it did stop the navigation, but it stopped the actual action as well. Clicking on mark as read did nothing.

sfdye commented 5 years ago

Any progress on this issue please?

tanmayrajani commented 5 years ago

So, instead of letting form action handle the POST request, I tried to mimic the action by using fetch, it's performing the action but still redirecting to same page and not rendering somehow.

I'm passing redirect: 'error' to fetch, I think it's supposed to stop the redirection but it's not.

Here's the snippet of what I tried:

const forms = select.all(('.NPG-dropdown form'));
for (const form of forms) {
    const action = form.getAttribute('action');
    const method = form.getAttribute('method');

    form.addEventListener('submit', async () => {
        const formData = new FormData();
        formData.append('utf8', '✓');
        formData.append('authenticity_token', select('input[name="authenticity_token"]', form).value);

        // This fails
        await fetch(location.origin + action, {
            method,
            redirect: 'error',
            body: new URLSearchParams(formData)
        });
        updateLoop();
    });

    form.removeAttribute('action');
}

If anyone got a hint or want to try out it's pushed to action-stay-same-page branch

fregante commented 5 years ago

You can use this: https://github.com/sindresorhus/refined-github/blob/master/source/libs/post-form.ts

And then something like

delegate('buttons', 'click', async event => {
    event.preventDefault();
    await postForm(event.delegateTarget.form);
    button.closest('.unread').classList.replace('unread', 'read')
});

Delegate is https://github.com/fregante/delegate-it

sfdye commented 5 years ago

No longer staying on page after marking as read seems to be fixed by 1.6.0, however the read item does not disappear until I manually refreshed page.

tanmayrajani commented 5 years ago

It wouldn't go away until the notification popover is open. Once you close it by clicking elsewhere, it seems to go away for me.

Here's how it happens: output

sfdye commented 5 years ago

@tanmayrajani is this the old behavior? feels a bit different for me

tanmayrajani commented 5 years ago

@sfdye honestly, I don't remember the old behaviour. It wasn't working since long :mask:

Although, if it goes away, you wouldn't have the option to mark as unread/unmuted if you've accidentally clicked on it, right?

sfdye commented 5 years ago

@tanmayrajani Cool, makes sense. Thanks a lot for the fix and release!