APP-672 New Keyboard shortcuts #30
Conversation
|
Jira ticket: APP-670 MSQuill Spike |
|
@bholloway This nit change from the previous set of commits broke keyboard shortcut registration. There's no simple way to detect when this object will init, so checking on registration is the quickest fix. |
kvnvelasco
force-pushed
the
APP-670-MsQuill-Spike
branch
from
87debc7 to
4fed466
Compare
* Add mixed fraction type * Added definite integral command
bholloway
left a comment
bholloway
left a comment
There was a problem hiding this comment.
Mostly nits but a couple of things I think worth blocking on
src/services/commands.js
Outdated
| var commands = options.commands || []; | ||
| var ignoredCharacters = options.ignoredCharacters || []; | ||
| // We're going to create an index for ignored character lookup | ||
| ignoredCharacters.forEach(char => this.cursor.grammarDicts.ignoredCharacters[char] = true); |
There was a problem hiding this comment.
nit the assignment return is generally considered confusing but it seems to be common practice in this code base
change probably best just to avoid the lambdas since they aren't used anywhere else
| var boundSymbol = binder(symbolDefinition); | ||
| var boundSymbol; | ||
| if (symbolDefinition.useInternalSymbolDef) boundSymbol = LatexCmds[symbolDefinition.name]; | ||
| else boundSymbol = binder(symbolDefinition); |
There was a problem hiding this comment.
opinion a ternary would make this assignment more declarative
| var latexWithoutBs = symbolDefinition.latex.replace('\\', ''); | ||
| symbols[latexWithoutBs] = boundSymbol; | ||
| } | ||
| if(symbolDefinition.htmlEntity) |
There was a problem hiding this comment.
nit space between if and parentheses.
kvnvelasco
changed the title
kvnvelasco
force-pushed
the
APP-670-MsQuill-Spike
branch
from
0b55a02 to
a760607
Compare
bholloway
left a comment
bholloway
left a comment
There was a problem hiding this comment.
I think this is good to go. Only one nit. Last call.
| var ignoredCharacters = options.ignoredCharacters || []; | ||
| // We're going to create an index for ignored character lookup | ||
| ignoredCharacters.forEach(char => this.cursor.grammarDicts.ignoredCharacters[char] = true); | ||
| ignoredCharacters.forEach(function(char) { this.cursor.grammarDicts.ignoredCharacters[char] = true }.bind(this)); |
There was a problem hiding this comment.
nit Complexity is growing here. I think this probably makes more sense as a for loop at this point.
bholloway
changed the title
BrandonNav
left a comment
BrandonNav
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 2 of 6 files at r2, 4 of 4 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @bholloway and @kvnvelasco)
BrandonNav
left a comment
BrandonNav
left a comment
There was a problem hiding this comment.
Dismissed @bholloway from 7 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @kvnvelasco)
This change is