TNL-731: Add possibility to search notes. - #6086
Conversation
45c0465 to
f971081
Compare
12967a2 to
0f32e62
Compare
There was a problem hiding this comment.
Why this check is needed if this tab must be closable?
There was a problem hiding this comment.
Why this check is needed if this tab must be closable?
It's a base class for RecentActivity (is not closable) and SearchResults (is closable) tabs.
But I'll remove this check to be more informative if somebody will try to close non-closable tab.
There was a problem hiding this comment.
Why this check is needed if this tab must be closable?
done.
f971081 to
9c559dc
Compare
ce7bae1 to
218aeac
Compare
There was a problem hiding this comment.
Please remove set().
Removed.
|
@olmar all your comments are addressed. |
There was a problem hiding this comment.
@polesye Please do not use _ as a prefix for private variables, as it can easily be confused with UnderscoreJS constructs. For example later on, you have a this._times[label] = now() whose syntax is somehow similar to _.times.
There was a problem hiding this comment.
Please do not use _ as a prefix for private variables, as it can easily be confused with UnderscoreJS constructs. For example later on, you have a this._times[label] = now() whose syntax is somehow similar to _.times.
Sure. Fixed ;).
|
@polesye Review finished, as far as I'm concerned, good to merge into feature branch once my comments have been addressed and tests pass. |
|
@jmclaus all comments are addressed. |
|
@polesye 👍 Once tests pass. |
There was a problem hiding this comment.
ypo: to Search results
Fixed typo.
|
Very thorough work. Couple of nitpicks, but overall 👍 |
4 participants
Ticket: TNL-731
sandbox: http://tymofij.m.sandbox.edx.org/courses/edX/DemoX/Demo_Course/edxnotes/
@explorerleslie,
@tymofij , @olmar , @jmclaus please review.