Skip to content

Add support for avatar rewards ( + fix date overlapping with title) #201

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

Merged
merged 3 commits into from
Mar 26, 2021

Conversation

An0n3m0us
Copy link
Contributor

@An0n3m0us An0n3m0us commented Mar 3, 2021

Closes #200

image

  • Fix date overlapping with title
  • Add support for avatar rewards (thumbnail, content)
  • Fix URL for avatar rewards (uses me instead of KAID or username which interrupts redirect)

Copy link
Member

@JettBurns14 JettBurns14 left a comment

Choose a reason for hiding this comment

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

Could you include screenshots/pasted HTML from the popup notifications before and after this fix? Also, could you check if these notifications have content values or not in the API request the extension sends? That'll give me more context.

@An0n3m0us
Copy link
Contributor Author

An0n3m0us commented Mar 4, 2021

Could you include screenshots/pasted HTML from the popup notifications before and after this fix? Also, could you check if these notifications have content values or not in the API request the extension sends? That'll give me more context.

Oh, you're completely correct; I forgot to test this without the extension.

image

It's missing a lot from the notifications API.
I'll try and continue work on this soon.

@JettBurns14
Copy link
Member

I'll see about getting my local extension running again and testing your fix, and possibly adding my own. I'll probably be on the KAE Discord tomorrow if you want to talk details there. Thanks for contributing!

@An0n3m0us An0n3m0us changed the title Fix date overlapping with title Add support for avatar rewards ( + fix date overlapping with title) Mar 4, 2021
@An0n3m0us
Copy link
Contributor Author

An0n3m0us commented Mar 4, 2021

Could you include screenshots/pasted HTML from the popup notifications before and after this fix?

I couldn't work out how to do it first, but here it is xD

CURRENT <--- ---> NEW
image

(This is for the two avatar rewards shown in the screenshot in the original post)

@An0n3m0us
Copy link
Contributor Author

An0n3m0us commented Mar 4, 2021

There is a small issue;
The URL needs to be modified for the rewards notification.

Lets take the Hopper reward notification shown in the original post.
In the API entry, the URL is like this:
/profile/me?show_avatar_customizer=1&selected_avatar_part=hopper_jumping_style

But using /me does not seem to redirect to the badge as it's busy redirecting to the users profile.
So the original redirect is ignored.

Instead, using either the KAID or username will work.
Code for this should be added to the href item in the NotifElm object.

Unfortunately I cannot seem to use notif.url.replace to replace the me part with notif.kaid.

@An0n3m0us
Copy link
Contributor Author

An0n3m0us commented Mar 4, 2021

I have managed to fix it now, but I got this error on the first build:

ERROR in ./src/popup.ts
./src/popup.ts
[tsl] ERROR in ./src/popup.ts(139,97)
      TS2532: Object is possibly 'undefined'.

But after running the build command again the error does not show.
So is the error existent or not?

EDIT: Nevermind; I worked out what was causing the error.

The redirect link is fixed now.

@JettBurns14
Copy link
Member

Very cool! I won't be on Discord today after all, got busy, but likely will tomorrow, and will see about testing this change then.

@An0n3m0us
Copy link
Contributor Author

An0n3m0us commented Mar 5, 2021

No problem! I would not be available anyway as Europe is 6-8 hours ahead of Central Time.
But if there are any details I need information on, I guess I can just post in DM/general chat and await your reply.

@An0n3m0us An0n3m0us mentioned this pull request Mar 5, 2021
3 tasks
@JettBurns14 JettBurns14 added enhancement New feature or request bug Something isn't working labels Mar 7, 2021
@An0n3m0us
Copy link
Contributor Author

An0n3m0us commented Mar 7, 2021

Could you include screenshots/pasted HTML from the popup notifications before and after this fix?

#201 (comment)

Also, could you check if these notifications have content values or not in the API request the extension sends?

It seems not. Here are the API entries for the notifications that I have shown in the original post:
https://www.dropbox.com/s/jjwog2mkl53kkhx/notifications.json?dl=0

@JettBurns14
Copy link
Member

JettBurns14 commented Mar 7, 2021

@An0n3m0us Looks good to me. It'd probably be easier if I merge this before #211, then you update that branch accordingly. Sound good?

An0n3m0us added a commit to An0n3m0us/ka-extension-ts that referenced this pull request Mar 20, 2021
@An0n3m0us An0n3m0us requested a review from JettBurns14 March 24, 2021 20:23
@MatthiasPortzel MatthiasPortzel self-requested a review March 26, 2021 20:16
@MatthiasPortzel MatthiasPortzel merged commit 8b1f68e into ka-extension:master Mar 26, 2021
@An0n3m0us An0n3m0us deleted the fixnotifbug branch March 30, 2021 04:29
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reward notification date overlaps with reward title
3 participants