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
Log search #1114
Log search #1114
Conversation
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
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.
It appears to include the timestamps in the search even if the timestamps are not shown (I searched for '2'). This is confusing when clicking next as the orange highlight disappears.
<Icon | ||
material="close" | ||
tooltip={_i18n._(t`Clear`)} | ||
onClick={() => setSearch("")} | ||
/> |
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 guess it doesn't work keeping it inside the SearchInput. It might fit better to the left of the up/down arrows if not in the SearchInput?
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 follow the usual rules for placing elements in the search, such as native search in the browser.
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 see. I didn't make the connection because the input field is delineated by a rectangle whereas the native search in the browser delineates the whole search tool by a rectangle, and separates the input field from the "prev next clear" buttons with just a vertical line
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>
Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>
Signed-off-by: Lauri Nevala <lauri.nevala@gmail.com>
* @returns A react element with a row itself | ||
*/ | ||
getLogRow = (rowIndex: number) => { | ||
const isSeparator = this.logs[rowIndex] === "---newlogs---"; // TODO: Use constant separator |
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.
Why is "---newlogs---" within the logs list itself? It seems very weird that if I search of "new" it would find this line. Is there a problem with keeping an index?
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.
Removing separator stuff from this PR
import { TableRowProps } from "../table/table-row"; | ||
import { ItemObject } from "../../item.store"; | ||
import throttle from "lodash/throttle"; | ||
import debounce from "lodash/debounce"; | ||
import isEqual from "lodash/isEqual"; | ||
import ResizeSensor from "css-element-queries/src/ResizeSensor"; | ||
|
||
interface Props { | ||
items: ItemObject[]; | ||
interface Props<T extends ItemObject = any> { |
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.
Does defaulting this to any
gain us anything. I believe that this is a code smell.
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.
It does, since ItemObject
contains 2 specific methods getId()
& getName()
which is related to workspaces. But from now on we're using virtual list for any kind of lists, so this line helps to avoid typescript errors.
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.
expect(searchStore.occurrences).toEqual([]); | ||
}) | ||
|
||
it("doesn't brake if no text provided", () => { |
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.
break?
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.
Of course.
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
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.
LGTM
@aleksfront @nevalla integration tests are still failing. |
Yes, we need to fix extensions loading in |
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
@aleksfront Please try to rebase with |
@nevalla It's green now! 🚀 |
Signed-off-by: Alex Andreev <alex.andreev.email@gmail.com>
Awesome, thanks, guys! When it's gonna be released? |
Part of #430
CmdOrCtrl+F