As far as UI goes, the delete button (imo) should be smaller and placed under the ‘by user’ section. It’s less of an eyesore that way with two big buttons in such close proximity.
Anachronist said...Looking good,HTML tags would definitely make for a slightly faster release but they aren't very self-explanatory so I might end up adding buttons.
One question, what are you using for the markup in the text entry field? are we adding in our own HTML tags or are we getting some basic buttons for markup?
Do you have an ETA on when the extension will be available, bugs and all? I will happily be part of the test group.
I currently add all my own mnemonics into the Notes section. It would be great to start adding them into this instead.
Looking forward to using this one.
It should be complete within the next couple days but I can't predict whether or not bugs will affect this.
Saphrose said... As far as UI goes, the delete button (imo) should be smaller and placed under the 'by user' section. It's less of an eyesore that way with two big buttons in such close proximity.Alright, good idea. I only left it big because the text changes to "Really Delete?" as a confirmation when clicked but I can just fix this with a smaller font size.
Nice update! Approx when can we see it coming?
gregorspv said... Nice update! Approx when can we see it coming?Probably sometime between later today and Monday but, like I've said before, I can't say for sure.
This looks good, I am looking forward to it.
I keep refreshing this page to see if there are any updates haha. Looking forward to it !
aarondes said... I keep refreshing this page to see if there are any updates haha. Looking forward to it !It could very well be released by tonight. There's only one thing I know of that I have to fix before it's ready.
It still has some bugs but , since I’m going to bed, I’m going to post a download here for anyone that just can’t wait to try this. Make sure to update frequently. If you get “undefined” everything after deleting a mnemonic, just click the next arrow and this will fix.
https://greasyfork.org/en/scripts/7954-wk-community-mnemonics
hey so is it only for vocab? because it’s not working for kanji at all… i’m guessing you haven’t added it yet.
Nice! Thank you for the time you’re putting into it!
ShotgunLagoon said…It works for me on both kanji and vocab using Firefox.
hey so is it only for vocab? because it’s not working for kanji at all… i’m guessing you haven’t added it yet.
Great idea!
I’m just looking over the code a bit and I have some feedback:
- jQuery’s .bind() method is deprecated in favor of .on() (probably because ES5 has a native .bind() method that’s completely unrelated)
- Though I didn’t try it, but I doubt this works on Greasemonkey. You didn’t @grant unsafeWindow (and just used window as well instead) and Greasemonkey doesn’t like when you use the window’s jQuery for adding event listeners and stuff
- I noticed some ternary expressions like “.inArray( CMUser, curMUsers )) > -1" since that inequality is going to return a boolean already, it doesn't need to be in a ternary</li><li>I think it's kind of funny that you encoded the naughty words regex so they don't appear plainly in the script :D</li><li>You should use // @run-at document-end instead of using (document).ready()
- CMIsReview and CMIsLesson are declared without a ‘var’ statement. This isn’t going to break anything since you’re not using strict mode though.
- If you’re just setting the text of a node instead of inserting a string of HTML, use (element).text(text) instead of (element).html(text). It’s faster and safer (it auto-encodes special html characters)
- (element).is(selector) is great (returns true/false if the element matches a css selector). You're doing a lot of "if ((something).parent().attr(‘id’) == ‘some_id’)”, but you could replace that with if((something).parent().is('#some_id")). You could do the same thing where you use (el).attr(‘class’) == ‘some class’
- Store your jQuery elements so you don’t have to select elements every single time you use them. Much more maintainable and easier to read.
- There’s a lot of ternary expressions and other things where you have the same thing pasted multiple times (like “((isMeaning) ? “m” : “r”)”) Store the result as a variable and reuse that. (“DRY” programming - “Don’t Repeat Yourself”, makes things so much more maintainable and easier to read).
- Nested ternary expressions are really hard to read, and just because you can use a ternary doesn’t mean you should
- I noticed you have an else { with an nested if(){ in it, just make that an else if (condition)
- Use smaller, descriptively named functions when you can, loadCM is like 400 lines!
- I noticed for functions like your $.ajax callbacks have three arguments passed in, but since you’re not using them, you don’t need to include them. Javascript doesn’t require the number of arguments in a function declaration to match the number of arguments you’re calling it with.
- Use line breaks, man! Really long text is hard to read. It’s going to be a nightmare to maintain if you come back to this after a while.
Most importantly though, this is very vulnerable to XSS and I would caution people against using this at the moment. You’re not checking to make sure a mnemonic doesn’t contain script tags or a link with a ‘javascript:something()’ href. At the least you should be checking before posting a mnemonic, AND before adding it to the DOM. Both places are necessary because someone could easily tamper with the script (or post directly to the spreadsheet) to allow them to submit a mnemonic with code in it, and unless you’re checking before appending it to the page, it’s going to spit out their compromised mnemonic on the page and it can be executed.
Here’s one possible solution (there’s a much better one after this, this is just provided as an example):
You should add a call to checkCMHTMLTags() in the function where you append community mnemonics to the DOM. Also, in your checkCMHTMLTags() you could add this before your ‘for’ loop:
if (/<(a|script)/gi.test(text)) {
return false;
}
Which returns false if ‘<a’ or ‘<script’ (case insensitive) appears anywhere in the text.
Even better than that would be to whitelist just the tags you want to allow, if it contains anything that has a ‘<’ or ‘</’ and isn’t immediately followed by b/i/u/s/span, return false. Whitelisting is way better than blacklisting (read the ‘Blacklisting’ section of this page to see why blacklisting isn’t so great).
I’m not an expert at preventing XSS, so I suggest you read up on good security practices.
Since this runs off Google Docs and you can’t properly sanitize user input on the back end like you could if you ran it using your own personal server, you need to be extremely careful with how you treat user input. It only takes one person to ruin everything. Remember, the first rule of development is never trust user input!
Good start, but definitely needs some work. Now that I’ve spent way too much time code reviewing this and it’s nearly 3AM, I need to go to bed…
眠いだよ
great effort. looks interesting, but i personal change all the radicals to their proper names, so i imagine all the community mnemonics. that’s if everyone is using the horrendous radical names wk uses. would be great if this could branch off for those who use proper radicals…
also, this would be GREAT for the “Context Sentences”. wk’s context sentences are all completely useless (too complicated and too puerile and pretentious).
Nice work on building this. I think it’s a great feature, and will give it a shot tomorrow.
Given that you’re getting code reviews in the forum if you wanted to Github it, I’m sure WK community members would be happy to do PRs for improvements / ideas. Thanks!
Did someone mess with the spreadsheet? All existing mnemonics were overwritten and the script shouldn’t do that. I need to know whether or not this happened with the script or self because this happening when the script is popular won’t be good.
Edit: This happened again. It should read the spreadsheet and know how long it is and not overwrite existing items but obviously this isn’t working properly for someone. I need to know how this is happening before I can fix it.
sheodox said…Great idea!
I’m just looking over the code a bit and I have some feedback:
…
Good start, but definitely needs some work. Now that I’ve spent way too much time code reviewing this and it’s nearly 3AM, I need to go to bed…
眠いだよ
- jQuery’s .bind() method is deprecated in favor of .on() (probably because ES5 has a native .bind() method that’s completely unrelated)
- Though I didn’t try it, but I doubt this works on Greasemonkey. You didn’t @grant unsafeWindow (and just used window as well instead) and Greasemonkey doesn’t like when you use the window’s jQuery for adding event listeners and stuff
- I noticed some ternary expressions like “.inArray( CMUser, curMUsers )) > -1" since that inequality is going to return a boolean already, it doesn't need to be in a ternary</li></ul>Okay, good to know.<br><ul><li>I think it's kind of funny that you encoded the naughty words regex so they don't appear plainly in the script :D<br></li></ul>I would rather not even look at them myself.<br><ul><li>You should use // @run-at document-end instead of using (document).ready()
- CMIsReview and CMIsLesson are declared without a ‘var’ statement. This isn’t going to break anything since you’re not using strict mode though.
- If you’re just setting the text of a node instead of inserting a string of HTML, use (element).text(text) instead of (element).html(text). It’s faster and safer (it auto-encodes special html characters)
- (element).is(selector) is great (returns true/false if the element matches a css selector). You're doing a lot of "if ((something).parent().attr(‘id’) == ‘some_id’)”, but you could replace that with if((something).parent().is('#some_id")). You could do the same thing where you use (el).attr(‘class’) == ‘some class’
- Store your jQuery elements so you don’t have to select elements every single time you use them. Much more maintainable and easier to read.
- There’s a lot of ternary expressions and other things where you have the same thing pasted multiple times (like “((isMeaning) ? “m” : “r”)”) Store the result as a variable and reuse that. (“DRY” programming - “Don’t Repeat Yourself”, makes things so much more maintainable and easier to read).
- Nested ternary expressions are really hard to read, and just because you can use a ternary doesn’t mean you should
- I noticed you have an else { with an nested if(){ in it, just make that an else if (condition)
- Use smaller, descriptively named functions when you can, loadCM is like 400 lines!
- I noticed for functions like your $.ajax callbacks have three arguments passed in, but since you’re not using them, you don’t need to include them. Javascript doesn’t require the number of arguments in a function declaration to match the number of arguments you’re calling it with.
- Use line breaks, man! Really long text is hard to read. It’s going to be a nightmare to maintain if you come back to this after a while.
if (/<(a|script)/gi.test(text)) {
return false;
}
I implemented this in the newest version (0.9.0.3)
I’m not a Javascript expert by any means but I’ve written several userscripts for Greasemonkey that use unsafeWindow, and I’ve never heard of “@grant unsafeWindow”.
Also “@run-at document-end” makes your script execute when the document has loaded, but that is the default value anyway. I never use any “onready” callbacks etc. in my userscripts, all the code you put in it will only be run after the document is ready.
I’ll check this out later by the way, could be really nice 
Bugs I’ve found so far since I’ve released this:
- Backspace moves cursor to end when format buttons are used when entering mnemonics within reviews and lessons
- When adding items to the spreadsheet, sometimes it gets the index wrong and overwrites other items
- Voting on reading mnemonics affects the spreadsheet and not the user’s local storage (fixed in 0.9.0.4)
- For some people (only one that I’ve seen), the script make cause the page to freeze in reviews (and possibly lessons)
Samuel-H said... Did someone mess with the spreadsheet? All existing mnemonics were overwritten and the script shouldn't do that. I need to know whether or not this happened with the script or self because this happening when the script is popular won't be good.Hmm, well if it works in Firefox, no big deal. I've just had lots of issues with scripts breaking for stuff like that, I wonder what's different about what I was doing.
Edit: This happened again. It should read the spreadsheet and know how long it is and not overwrite existing items but obviously this isn't working properly for someone. I need to know how this is happening before I can fix it.sheodox said...Great idea!
I'm just looking over the code a bit and I have some feedback:
...
Good start, but definitely needs some work. Now that I've spent way too much time code reviewing this and it's nearly 3AM, I need to go to bed...
眠いだよ
I didn't know this. I will change them eventually.
- jQuery's .bind() method is deprecated in favor of .on() (probably because ES5 has a native .bind() method that's completely unrelated)
I tested it on Firefox with Greasemonkey and it worked fine. Does it affect anything if I leave this out?
- Though I didn't try it, but I doubt this works on Greasemonkey. You didn't @grant unsafeWindow (and just used window as well instead) and Greasemonkey doesn't like when you use the window's jQuery for adding event listeners and stuff
Okay, good to know.
- I noticed some ternary expressions like "$.inArray( CMUser, curMUsers )) > -1" since that inequality is going to return a boolean already, it doesn't need to be in a ternary
I would rather not even look at them myself.
- I think it's kind of funny that you encoded the naughty words regex so they don't appear plainly in the script :D
I'm unfamiliar with this method. What's better about it?
- You should use // @run-at document-end instead of using $(document).ready()
They are global variables like CMData and CMVotes.
- CMIsReview and CMIsLesson are declared without a 'var' statement. This isn't going to break anything since you're not using strict mode though.
Good to know.
- If you're just setting the text of a node instead of inserting a string of HTML, use $(element).text(text) instead of $(element).html(text). It's faster and safer (it auto-encodes special html characters)
I hadn't heard of that one. I will do this from now on.
- $(element).is(selector) is great (returns true/false if the element matches a css selector). You're doing a lot of "if ($(something).parent().attr('id') == 'some_id')", but you could replace that with if($(something).parent().is('#some_id")). You could do the same thing where you use $(el).attr('class') == 'some class'
This was because of my laziness in not really spending too much time on learning how jQuery variables work because I always have problems with them when I try to use them.
- Store your jQuery elements so you don't have to select elements every single time you use them. Much more maintainable and easier to read.
I sometimes just couldn't think of a decent name for the "m" or "r" and didn't create a variable for just that reason.
- There's a lot of ternary expressions and other things where you have the same thing pasted multiple times (like "((isMeaning) ? "m" : "r")") Store the result as a variable and reuse that. ("DRY" programming - "Don't Repeat Yourself", makes things so much more maintainable and easier to read).
I agree, I probably use them more than I should. I like to minimize lines when I can but I should probably prefer readability.
- Nested ternary expressions are really hard to read, and just because you can use a ternary doesn't mean you should
It would be good to know where this is.
- I noticed you have an else { with an nested if(){ in it, just make that an else if (condition)
Yeah, I probably should do that...
- Use smaller, descriptively named functions when you can, loadCM is like 400 lines!
That code was from a reference so I just didn't mess with it in case it would affect anything.
- I noticed for functions like your $.ajax callbacks have three arguments passed in, but since you're not using them, you don't need to include them. Javascript doesn't require the number of arguments in a function declaration to match the number of arguments you're calling it with.
I have a big screen resolution (2160 x 1440) and most lines are within my screen width.
- Use line breaks, man! Really long text is hard to read. It's going to be a nightmare to maintain if you come back to this after a while.
if (/<(a|script)/gi.test(text)) {
return false;
}
I implemented this in the newest version (0.9.0.3)
About the run-at, here's a link to the GM Docs. Essentially it lets you says when your script should run in relation to the loading of a page. Document-end is the same as binding your own onload event listener, you just don't need to bind anything. As looki said though, this document-end is the default setting if none is specified.
Yeah I know those are globals, and it won't hurt anything leaving them without a var. If you used 'var' all your usages would still work fine, since it's all within the same closure. JS variables are scoped to within the nearest function, if you make a variable inside a function, it'll be available anywhere inside that function and any nested functions inside of it. Since the variables are all declared in a scope above all the functions they're usable everywhere. I think what you're thinking of though, is that if you make a variable inside a function without 'var', it creates it in the global scope instead of that function's scope.
There's a few places the else{ if{ stuff is found, around like 600 and 620ish for example
There's not really a point to minimizing lines, especially on Javascript that's stored locally (vs always downloaded, in which case you would use minification strategies).
For storing jQuery elements, it's easy:
var $button = $('#some-button-id');
$button.css('background', 'red');
(Just as a convention, I like naming jQuery variables starting with a $ so I know what they are.)
Whenever you write the same code twice, or copy/paste, if you ever have to change that, it is super annoying to do. If you do the same thing more than once, use a function and call it in both places you need it.
I thought of another strategy that would make this more secure and user friendly. Instead of having straight up HTML in the mnemonics, create some sort of markup syntax (like Markdown for example, used by lots of places, including reddit). You can do the same checks before posting a url to try to prevent script and anchor tags from being submitted, but before appending the html to the DOM (and only then, don't do for submissions) do this:
- Encode all html special characters (a process that changes stuff like < to > so it appears as '<' on the page, instead of being interpreted as an opening character for an HTML tag)
- Transform whatever markup syntax you use into the actual HTML
sheodox said...There's a few places the else{ if{ stuff is found, around like 600 and 620ish for exampleOkay, I'll try to fix these where I find them.
For storing jQuery elements, it's easy:Thank you, I will use this from now on.
var $button = $('#some-button-id');
$button.css('background', 'red');
(Just as a convention, I like naming jQuery variables starting with a $ so I know what they are.)
Whenever you write the same code twice, or copy/paste, if you ever have to change that, it is super annoying to do. If you do the same thing more than once, use a function and call it in both places you need it.
I thought of another strategy that would make this more secure and user friendly. Instead of having straight up HTML in the mnemonics, create some sort of markup syntax (like Markdown for example, used by lots of places, including reddit). You can do the same checks before posting a url to try to prevent script and anchor tags from being submitted, but before appending the html to the DOM (and only then, don't do for submissions) do this:This is something I might do after I fix the glitches. Right now there's a bad one where new items are written to the first index of the sheet, replacing other items and their mnemonics. I can't seem to replicate this myself but I think it may possibly be the result of someone receiving the error that the table couldn't be read and continuing without refreshing (I don't know for sure, though).This way, you can make it easier for people to use in mnemonics, like only requiring them to do [k|感] or something to denote a kanji instead of them writing (or at least seeing) a big string of HTML, you can transform [k|感] into <span class='highlight-kanji'>感</span> when you put it on the page instead. So it's easier for users to use, and any HTML characters the users put in would be escaped and not interpreted as HTML so you'd be safe from XSS. The only HTML that would be inserted into the document would be the HTML you get from transforming from the markup, since any HTML used by users would be encoded (and not interpreted as HTML) anyway.
- Encode all html special characters (a process that changes stuff like < to > so it appears as '<' on the page, instead of being interpreted as an opening character for an HTML tag)
- Transform whatever markup syntax you use into the actual HTML
I can understand the advantages of using GDocs instead of your own backend/server, but I think it might be way too limited/insecure for this purpose.
dothack said... I can understand the advantages of using GDocs instead of your own backend/server, but I think it might be way too limited/insecure for this purpose.I haven't encountered any problems with this. There is a limit of 10 mnemonics per item meaning and reading and 5000 characters in a cell doesn't seem to be an issue. I can also recover old versions in case someone tries to sabotage the spreadsheet (and why would someone do this anyway).