-
Notifications
You must be signed in to change notification settings - Fork 681
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
Add custom highlights #425
Conversation
@@ -227,6 +227,10 @@ $(function() { | |||
var chan = chat.find(target); | |||
var msg; | |||
|
|||
if (highlights.some((h) => data.msg.text.indexOf(h) > -1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this support regex? Maybe if it starts with /
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels a bit unnecessary to include ES6 syntax - and effectively breaking 45 % of global browser market share - for something so small. Could be an opportunity to add babel transpilation though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove it for now, but I would really want to add babel at some point. We can use arrow functions on the server, but not the client, and it seems a massive shame to leave out such nice development tools just because rubbish browsers.
I have a little bit of an obsession with arrow functions just now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree with that, @YaManicKill. Just not worth breaking browsers for a couple fat arrow functions just now :-)
@@ -504,6 +511,10 @@ $(function() { | |||
if (name === "userStyles") { | |||
$(document.head).find("#user-specified-css").html(options[name]); | |||
} | |||
if (name === "highlights" ) { | |||
var highlightString = options[name]; | |||
highlights = highlightString.split(",").map((h) => h.trim()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above re: ES6 on the client.
5ebfd09
to
0c8aad0
Compare
Fixed all comments, including the fun bug where if you didn't put any custom highlights, you were notified for everything. |
0c8aad0
to
9f3d690
Compare
I find it a little weird that we now have highlights on both the client and the server, and it won't work very well in the future when we add push notifications and such. But I'll call it good enough for now so have a 👍 |
|
||
<div class="col-sm-12"> | ||
<label class="opt"> | ||
<label for="highlights" class="sr-only">Custom Highlights (separate by ",")</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest the following placeholder here and in the <input />
below:
Custom highlights (comma-separated keywords)
Reasoning is: no upper-casing of "highlight" to match other inputs/labels in this page, and using words instead of characters as 1) screen readers (at least VoiceOver) will not read them at all (it just reads "separated by") and the ','
looks weird, very packed on the default font.
I agree with @maxpoulin64 but oh well, we'll refactor later :-) |
I agree with both of you, but the only way to sort it out would be to put it on the server which is a bigger issue in itself. Will address comments tomorrow. |
For sure! I think it was more of a general comment. I'm happy with the way you took here overall. |
We will be able to move it to the server after @maxpoulin64's settings overhaul. |
@xPaw Yeah, the only problem with that is that everything is an option to sync to the server, we would have to require custom highlights being synced for that to work. Is that something we want to do? I'm not sure, open to ideas. |
Hey @YaManicKill, any news on my 2 minor comments? :-) |
I've been busy, I'll hopefully get that sorted tonight or tomorrow. |
9f3d690
to
feda661
Compare
Done :-) |
Awesome @YaManicKill, great job! 👍 and merging, Travis CI seems to have a hard time with the Mac builds... |
…ghlights Add custom highlights
No description provided.