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

Fix NotificationBlock #2065

Merged
merged 6 commits into from
Jun 1, 2021
Merged

Fix NotificationBlock #2065

merged 6 commits into from
Jun 1, 2021

Conversation

nightpool
Copy link
Member

If anyone has feature requests, now is the time.

Comment on lines -42 to -49
try {
var m_blacklist = XKit.storage.get("notificationblock", "posts", "").split(",");
if (m_blacklist !== "") {
this.blacklisted = m_blacklist;
}
} catch (e) {
XKit.storage.set("notificationblock", "posts", "");
this.blacklisted = [];
Copy link
Member Author

@nightpool nightpool May 31, 2021

Choose a reason for hiding this comment

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

fun fact: because of the implicit parseInt in storage.get (https://github.com/new-xkit/XKit/blob/master/xkit.js#L437), i can't see how notificationBlock ever worked. Like, at all.

Copy link

@hobinjk hobinjk left a comment

Choose a reason for hiding this comment

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

This is great work! Always nice to review your code, learn something new every time! Functionality LGTM as well.

Only blocking issue I saw was that the icon doesn't support theming:

Screen Shot 2021-05-31 at 8 47 31 PM

@@ -1025,7 +1025,7 @@ XKit.extensions.xkit_patches = new Object({
}`, `xkit_interface_icon__${class_name}`);

if (typeof ok_icon !== "undefined") {
XKit.tools.add_css(`.${class_name} .xkit-interface-icon-completed {
XKit.tools.add_css(`.${class_name} .xkit-interface-completed {
Copy link

Choose a reason for hiding this comment

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

Wow, icon-completed was just never the right class huh

Extensions/notificationblock.js Outdated Show resolved Hide resolved
@nightpool
Copy link
Member Author

As i mentioned in the discord, i'm not sure how to resolve the theming issue, given that the notification block icon is currently a black png. if someone would like to recreate it as an SVG, i'd be happy to include that, but i currently do not want to spend/have the the time to spend doing so 😅

We could maybe do something simple with css filter: invert hueRotate but imagine the emotional toll.

@marcustyphoon
Copy link

It's not a great fix, but here's a grey icon version that's at least visible in dark mode/goth rave, like I did in #1997. It actually makes Postblock and Notificationblock kind of hard to tell apart, to be honest, but I dunno what we can easily do about that besides maybe having scratchyone do another #2054.

button_icon: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAABMAAAATCAYAAAByUDbMAAAAAXNSR0IArs4c6QAAAblJREFUOBGdk0soRFEcxi9NKUpWVmxYYoGU51KWhI2NZmNPsZpoNrPRpBHWrCSPslRsp4jFZMpKVhaKkGcJ4/fduWc6d+bOFf/65vv+z3POPWccJ8RisVgaHISU+FIRn4dDcw+USyQSx3BvcT7MLxlG8RH4ArV2I4vs4L+ySNSO2zpoWLVdYOlxdA5ErZhPVvq8352KsJK/DnNnceQ4mC4eHHTM4pogf4HgG0jZyX/tjAE6bo09SDrCdofgAbDITT0pKCPemleubjPaYveiqGsidkhvs465DfQMGkAUGMsaAZ8bTXOSRvci0MPEV4D7jHTMS69wkmS3p8tRnEGzSlL7Au2DRrAMHO1sC3QArbYKylk9g24ZUkfBPTDPJINOqkk7WwfvcrDOPJX+eoPmyDwAM0iFGXLPEpWIO3hDToilvdx8QM0Eu71W3DyNJfRnQKEJ9UmwsD70qQl6XAWfSLvDKNIlrClQzljdLNxl1TyiZ+gfU8wUSOsIVxIBtknDNwP3rJx2OEg8ZWKFYQT1EUfAjUlarA8vG82TcwH303Pm+S4VhskjmYXawS7Qf8+YvcAUdS3gwyQN/wCrsHgfRS7nugAAAABJRU5ErkJggg==",```

Copy link

@hobinjk hobinjk left a comment

Choose a reason for hiding this comment

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

Sorry, went to bed before circling back and approving

@nightpool
Copy link
Member Author

@marcustyphoon Okay, let's follow-up with the button icon fix in a separate PR (since we need at least two icons here, one with an "accent" color indicating that the post already has notifications blocked)

@nightpool nightpool merged commit 6abedbe into master Jun 1, 2021
@nightpool nightpool deleted the fix_notificationblock branch June 1, 2021 17:06
@marcustyphoon
Copy link

The other one is currently green, which seems fine as-is!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants