-
Notifications
You must be signed in to change notification settings - Fork 917
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
[Discover 2.0] Fixed recent query and Data Selector styles #7918
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,9 @@ | ||
.recentQuery__table { | ||
padding: $euiSizeXS; | ||
width: 1320px; | ||
border: $euiBorderThin; | ||
border-radius: $euiSizeXS; | ||
margin: 0 $euiSizeXS $euiSizeXS; | ||
|
||
thead { | ||
background-color: $euiColorLightestShade; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,60 +4,82 @@ | |
*/ | ||
|
||
import { BehaviorSubject } from 'rxjs'; | ||
import { DataStorage, Dataset } from '../../../common'; | ||
import { DataStorage } from '../../../common'; | ||
import { Query, TimeRange } from '../..'; | ||
|
||
// Todo: Implement a more advanced QueryHistory class when needed for recent query history | ||
const MAX_HISTORY_SIZE = 500; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make this a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call. Fast follow maybe? This was what Abby introduced in her PR but we don't today show any more than 10 to the user anyways, so having this value be user configurable isn't very useful yet. Once we implement more features for the table, then it makes sense adding it as an advanced setting |
||
export const HISTORY_KEY_PREFIX = 'query_'; | ||
|
||
export class QueryHistory { | ||
constructor(private readonly storage: DataStorage) {} | ||
private changeEmitter: BehaviorSubject<any[]>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think the type is just interface QueryHistoryItem {
time: number;
query: Query;
dateRange?: TimeRange;
}; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. imo i think the type should extend savedquery |
||
|
||
private changeEmitter = new BehaviorSubject<any[]>(this.getHistory() || []); | ||
constructor(private readonly storage: DataStorage) { | ||
this.changeEmitter = new BehaviorSubject<any[]>(this.getHistory()); | ||
} | ||
|
||
getHistoryKeys() { | ||
public getHistoryKeys(): string[] { | ||
return this.storage | ||
.keys() | ||
.filter((key: string) => key.indexOf('query_') === 0) | ||
.sort() | ||
.reverse(); | ||
.filter((key: string) => key.startsWith(HISTORY_KEY_PREFIX)) | ||
.sort((a, b) => { | ||
const timeA = parseInt(a.split('_')[1], 10); | ||
const timeB = parseInt(b.split('_')[1], 10); | ||
return timeB - timeA; // Sort in descending order (most recent first) | ||
}); | ||
} | ||
|
||
getHistory() { | ||
return this.getHistoryKeys().map((key) => this.storage.get(key)); | ||
public getHistory(): any[] { | ||
return this.getHistoryKeys() | ||
.map((key) => this.storage.get(key)) | ||
.sort((a, b) => b.time - a.time); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getHistoryKeys already sorts by time, is this sort needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. I added that sort afterwards. This is no longer needed |
||
} | ||
|
||
// This is used as an optimization mechanism so that different components | ||
// can listen for changes to history and update because changes to history can | ||
// be triggered from different places in the app. The alternative would be to store | ||
// this in state so that we hook into the React model, but it would require loading history | ||
// every time the application starts even if a user is not going to view history. | ||
change(listener: (reqs: any[]) => void) { | ||
public change(listener: (reqs: any[]) => void): () => void { | ||
const subscription = this.changeEmitter.subscribe(listener); | ||
return () => subscription.unsubscribe(); | ||
} | ||
|
||
addQueryToHistory(query: Query, dateRange?: TimeRange) { | ||
const keys = this.getHistoryKeys(); | ||
keys.splice(0, 500); // only maintain most recent X; | ||
keys.forEach((key) => { | ||
this.storage.remove(key); | ||
public addQueryToHistory(query: Query, dateRange?: TimeRange): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we just use the LRU library we have? |
||
const existingKeys = this.getHistoryKeys(); | ||
|
||
// Check if the query already exists | ||
const existingKey = existingKeys.find((key) => { | ||
const item = this.storage.get(key); | ||
return item && item.query.query === query.query && item.query.language === query.language; | ||
}); | ||
|
||
const timestamp = new Date().getTime(); | ||
const k = 'query_' + timestamp; | ||
this.storage.set(k, { | ||
if (existingKey) { | ||
// If the query exists, remove it from its current position | ||
this.storage.remove(existingKey); | ||
existingKeys.splice(existingKeys.indexOf(existingKey), 1); | ||
} | ||
|
||
// Add the new query to the front | ||
const timestamp = Date.now(); | ||
const newKey = `${HISTORY_KEY_PREFIX}${timestamp}`; | ||
const newItem = { | ||
time: timestamp, | ||
query, | ||
dateRange, | ||
}); | ||
}; | ||
this.storage.set(newKey, newItem); | ||
|
||
// Trim the history if it exceeds the maximum size | ||
if (existingKeys.length >= MAX_HISTORY_SIZE) { | ||
const keysToRemove = existingKeys.slice(MAX_HISTORY_SIZE - 1); | ||
keysToRemove.forEach((key) => this.storage.remove(key)); | ||
} | ||
|
||
// Emit the updated history | ||
this.changeEmitter.next(this.getHistory()); | ||
} | ||
|
||
clearHistory() { | ||
public clearHistory(): void { | ||
this.getHistoryKeys().forEach((key) => this.storage.remove(key)); | ||
this.changeEmitter.next([]); | ||
} | ||
} | ||
|
||
export function createHistory(deps: { storage: DataStorage }) { | ||
export function createHistory(deps: { storage: DataStorage }): QueryHistory { | ||
return new QueryHistory(deps.storage); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
.datasetConfigurator { | ||
height: 600px; | ||
height: 100%; | ||
} |
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.
maybe later, i remember user can config the format when date time is displayed, should this be read from user config?
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.
yeah from ui settings