Feature/#19 notifications tab title #31
No reviewers
Labels
No labels
dependencies
Priority: Critical
Priority: High
Priority: Low
Priority: Medium
State: Abandoned
State: Blocked
State: Completed
State: In progress
State: New
State: On Hold
State: Pending
State: Review needed
State: Waiting for release
Type: Bug
Type: Enhancement
Type: Maintenance
Type: Question
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: Serraniel/AniwatchPlus#31
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/#19-notifications-tab-title"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
I think the code how to get the notification count can be improved a bit more regarding its stability in case the user is logged out or there are no notifications.
Is 3 the correct index here? I just executed this via console and it wasn´t working correctly I think:
There probably also has to be a check if user is logged in and if there even is a notification I think.
You may try getting it via regular expression:
We should also insert a space after the number (at least the code I put in the other comment is without space). Also only add the space if there are notifications to insert.
Right now there is a space added after the number in line 16; however I'll take a look into retrieving the notificationCount via regex since its not always working on pathnames like /watch2gether
9fe2668c3aadds checks for logged in + notificationsConsole Error
NoTiFiCaTiOnCoUnT uNdEfInEd!seems to happen because runAfterLoad is called while the menu isn't visible..I just merged #24 which changed the
runAfterLoad. Maybe it behaves better now, if you update the branch but idk.Yeah I merged the commits from #24 into the branch for #19. Works a little bit better from what I saw in testing. I'll take a look at the merge conflicts and remove the logging later.
There still are conflicts. I think you used an older commit of develop to resolve them. I just will quickly do this also because of the build changes I did.
I think this looks good now. You can merge if you don´t see anything bad in the changes I did.
Considering the things mentioned, I'd suggest we either put this feature on hold until #42 gets resolved, or we use the old solution with the pathnameChange instead of the pushState event, or we use the new solution on /home and whenever the user opens his notifications (/notification) to update the NotificationCount. With this assigned to a variable we then just use the variable to insert the NotificationCount into the title on all the other pathnames.
@ -0,0 +7,4 @@}, ".*");core.runAfterLocationChange(() => {updateNotificationsInTitle();Getting the NotificationCount on every pushState event increases the chance of it failing. With this solution it fails for every item of the Community and Anime dropdownmenus. With the old solution it failed when inserting the Count into the title for the stats, jobs, seasonal, airing, random, profile, watchlist and notifications. 10 vs 8 pathnames where it doesn't work.
Furthermore with this solution we run into problems on the profile pathname:
We are adding the NotificationCount to the old count whenever we switch from one tab under profile to another; this did not happen with the old solution.
This seems to be related to #42 tho, if you enable Low-end mobile and check the pathnames after visting /home first, you'll see the notification count appear for a while and then disappear once the site is fully shown. With the new solution its definitely because of #42; we are trying to access the menu while the page isn't fully loaded.
We should rename this, if we keep using the pushState event.
I don't quite understand, why you moved this from helpers to core.
What would you suggest? Maybe location change? I think each pushstate also always could be a location change maybe?
The current helper file more was intended for JS helpers, not website helpers. But I agree that core already also is very mixed up between extension core and aniwatch core. This should be optimized in the feature I think.
@ -0,0 +7,4 @@}, ".*");core.runAfterLocationChange(() => {updateNotificationsInTitle();Do you remember how exactly you produced that bug with the number written in the title multiple times? I haven´t seen that yet.
But something else I noticed was the fact, the title seems to be updated by aniwatch after the blue loading bar on the top disappears. This should be investigated as a new opportunity to decide when we update the title and also would fix the current issue, that we just guess a random number to wait until location change before we update the title, which doesn´t always works as the following gif shows:
@ -0,0 +7,4 @@}, ".*");core.runAfterLocationChange(() => {updateNotificationsInTitle();Yeah thats what I meant. Looks like we should not only wait for the popState but also for onload once their preloader is fixed, or wait for the loading bar to disappear(?)
would be fine imo
@ -0,0 +7,4 @@}, ".*");core.runAfterLocationChange(() => {updateNotificationsInTitle();I introduced another observer, which will execute after the blue loading bar in the top disappears. This error seems not to appear any more with that specific change.
Renamed this with commit
a840f644b7@ -0,0 +7,4 @@}, ".*");core.runAfterLocationChange(() => {updateNotificationsInTitle();@ -0,0 +7,4 @@}, ".*");core.runAfterLocationChange(() => {updateNotificationsInTitle();I was able to fix this. We started our scripts after the preloader disappeared. We now also wait for the
document.readyStateto be"complete"so we can ensure everything is loaded correctly, as the preloader disappears to early on some pages.We can ignore the failing Travis builds for now, they are some kind of "global" issue in the project.
@kaffem if there are no objections by you I think we could merge this now.
This also fixes #42 .
Yes. That's it. Works good!