Fix NotificationBlock #2065
Fix NotificationBlock #2065
Conversation
| 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 = []; |
There was a problem hiding this comment.
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.
hobinjk
left a comment
hobinjk
left a comment
There was a problem hiding this comment.
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:
|
|
||
| if (typeof ok_icon !== "undefined") { | ||
| XKit.tools.add_css(`.${class_name} .xkit-interface-icon-completed { | ||
| XKit.tools.add_css(`.${class_name} .xkit-interface-completed { |
There was a problem hiding this comment.
Wow, icon-completed was just never the right class huh
|
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 |
|
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==",``` |
hobinjk
left a comment
hobinjk
left a comment
There was a problem hiding this comment.
Sorry, went to bed before circling back and approving
|
@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) |
|
The other one is currently green, which seems fine as-is! |
If anyone has feature requests, now is the time.