Conversation
assets/static/base.css
Outdated
| .tt-cursor, | ||
| .tt-suggestion:hover, | ||
| .tt-suggestion:focus { | ||
| div.tt-cursor, |
There was a problem hiding this comment.
This PR seems to contain changes from other unapproved PRs.
There was a problem hiding this comment.
Looks like I've based it on top of other branch, rather than the master
assets/static/filters.js
Outdated
| } | ||
|
|
||
| function renderHistory() { | ||
| const storage = window.localStorage; |
There was a problem hiding this comment.
Is it possible to pass localStorage here, that way you can use your mock without having to override window.localStorage?
There was a problem hiding this comment.
That is what I would call "a good idea" sir
There was a problem hiding this comment.
well, I could, but then I need to pass it through all the layers and mock it eventually anyway, so it doesn't really help much here IMHO, how strongly do you feel about it?
There was a problem hiding this comment.
It's an architectured dependency of this code, I'd really prefer if it could be provided, rather than assumed.
|
|
||
| })(); | ||
|
|
||
| Object.defineProperty(window, "localStorage", { |
There was a problem hiding this comment.
It doesn't look like this mock is working correctly, it seems to be undefined in tests.
There was a problem hiding this comment.
No, it needs to be required. Added some ugly tests around history
|
|
||
| function appendFilterToHistory(text) { | ||
| // require non empty text and enabled appends | ||
| if (!text || !appendsEnabled) return false; |
There was a problem hiding this comment.
It looks like this makes it impossible to clear the text stored in the storage engine.
There was a problem hiding this comment.
There's no clear method
| // final filter list we'll save to storage | ||
| var filterList = [ text ]; | ||
|
|
||
| // get current history list from storage and append it to our final list |
There was a problem hiding this comment.
You should consider using a Set here then.
assets/static/filters.js
Outdated
| // truncate the history to up to 11 elements | ||
| filterList = filterList.slice(0, 11); | ||
|
|
||
| storage.setItem(historyKey, filterList.join("\n")); |
There was a problem hiding this comment.
You'll have to marshal it back here. You might be interested in wrapping the storage engine to deal with marshalling tasks.
assets/static/filters.js
Outdated
| $(selectors.historyMenu).on("click", "a.history-menu-item", function(event) { | ||
| var elem = $(event.target).parents("li.history-menu"); | ||
| const historyArr = elem.find(".rawFilter").text().trim().split(","); | ||
| // we need to add filters one by one, this would reload alerts on every |
There was a problem hiding this comment.
Is it worth creating some sort of "unsee.transaction" then?
$(selectors.historyMenu).on("click", "a.history-menu-item", unsee.transaction(function(event) {
// append work here
}));
assets/static/filters.test.js
Outdated
|
|
||
| var elems = $("#historyMenu").find(".rawFilter"); | ||
| // foo is active so should only get default | ||
| expect(elems.length).toBe(1); |
There was a problem hiding this comment.
Would it be easier to use Jest's snapshot feature here (and again it the test below)?
There was a problem hiding this comment.
I wanted to use snapshots before, but didn't realized how easy it is, now I know
…nu to access it This allows to quickly select recently used filters from a dropdown. It also shows default (configured by unsee admin) filter and the saved one (saved by the user to a cookie)
Instead of assuming that window.localStorage is always used pass it via arguments
We only want unique values, Set is better for this
Show search icon for search history elements and provide a separate entries for the default and saved filter. Default & saved filter are always rendered on the bottom, if they are non-empty.
2 participants
…nu to access it
This allows to quickly select recently used filters from a dropdown. It also shows default (configured by unsee admin) filter and the saved one (saved by the user to a cookie)