-
Notifications
You must be signed in to change notification settings - Fork 98
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
Home table update #174
Home table update #174
Changes from 18 commits
9bd5b7e
97b09cb
78513c4
276a590
693b261
ba84e8d
f6cd878
e7783f8
e4d33d3
6487a5a
e1437d3
a7266dc
a38237b
0955eed
ac28702
d3d611f
a63dbcf
b228f5c
bda2115
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 |
---|---|---|
|
@@ -9,18 +9,19 @@ | |
* GitHub history for details. | ||
*/ | ||
|
||
import React, { useState, useRef, useEffect } from 'react'; | ||
|
||
import { | ||
EuiBasicTable, | ||
EuiSpacer, | ||
EuiLink, | ||
} from '@elastic/eui'; | ||
import React, { useState, useRef } from 'react'; | ||
|
||
import { EuiSpacer, EuiLink, EuiInMemoryTable, EuiIcon, EuiLoadingChart } from '@elastic/eui'; | ||
import { FILTER_OPTIONS } from '../../../../common/constants/explorer'; | ||
|
||
interface TableData { | ||
savedHistory: []; | ||
savedQuerySearch: (searchQuery: string, selectedDateRange: [], selectedTimeStamp, selectedFields: []) => void; | ||
savedQuerySearch: ( | ||
searchQuery: string, | ||
selectedDateRange: [], | ||
selectedTimeStamp, | ||
selectedFields: [] | ||
) => void; | ||
} | ||
|
||
export function Table(options: TableData) { | ||
|
@@ -31,7 +32,6 @@ export function Table(options: TableData) { | |
const pageSizeRef = useRef(); | ||
pageSizeRef.current = pageSize; | ||
|
||
|
||
const onTableChange = ({ page = {} }) => { | ||
const { index: pageIndex, size: pageSize } = page; | ||
|
||
|
@@ -40,44 +40,100 @@ export function Table(options: TableData) { | |
}; | ||
|
||
const columns = [ | ||
{ | ||
field: 'type', | ||
name: '', | ||
width: '3%', | ||
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. does a percentage width work well for an icon? could you try zoom in and zoom out the page to see if it still looks good 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. |
||
render: (item) => { | ||
if (item == 'Visualization') { | ||
return ( | ||
<div> | ||
<EuiLoadingChart size="m" /> | ||
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. Suggestion: This can be 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. It was Ani's choice to have a chart as icon for visualization, that's why went with this 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 this is specifically used for loading, is there a icon for this? 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. Yeah it was Ani's choice to have a chart as icon for visualization |
||
</div> | ||
); | ||
} else { | ||
return ( | ||
<div> | ||
<EuiIcon type="discoverApp" size="m" /> | ||
</div> | ||
); | ||
} | ||
}, | ||
}, | ||
{ | ||
field: 'data', | ||
name: 'Name', | ||
render: (item)=>{return <EuiLink onClick={() => | ||
{options.savedQuerySearch(item.query, [item.date_start, item.date_end], item.timestamp, item.fields)}}> | ||
{item.name} | ||
</EuiLink>}, | ||
align: 'left', | ||
render: (item) => { | ||
return ( | ||
<EuiLink | ||
onClick={() => { | ||
options.savedQuerySearch( | ||
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. Maybe a different name? the name savedQuerySearch is a little bit confusing, since it is just a function dispatching data. 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. It's actually dispatching data for the query search, thats why i used the same name. should i still change it? 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. Maybe it's fine, the dispatch will be removed once the changes to support passing object id gets merged. |
||
item.query, | ||
[item.date_start, item.date_end], | ||
item.timestamp, | ||
item.fields | ||
); | ||
}} | ||
> | ||
{item.name} | ||
</EuiLink> | ||
); | ||
}, | ||
}, | ||
{ | ||
field: 'description', | ||
name: 'Description', | ||
field: 'type', | ||
name: 'Type', | ||
align: 'right', | ||
}, | ||
]; | ||
|
||
|
||
|
||
let queryType = ''; | ||
const queries = options.savedHistory.map((h) => { | ||
const savedObject = h.hasOwnProperty('savedVisualization') | ||
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. if you need to check if it is savedVisualization more than once, you could do just const isSavedVisualization = h.hasOwnProperty('savedVisualization'), and then use isSavedVisualization in the following code. 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'm checking if its savedVisualization or not for each object, so it may differ for each entry of data. as i need to display the type of the query as a column, i need to populate it 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 mean in your each loop. 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. Got it |
||
? h.savedVisualization | ||
: h.savedQuery; | ||
const isSavedVisualization = h.hasOwnProperty('savedVisualization'); | ||
if (isSavedVisualization) { | ||
queryType = 'Visualization'; | ||
} else { | ||
queryType = 'Query'; | ||
} | ||
return { | ||
data: { | ||
name: savedObject.name, | ||
query: savedObject.query, | ||
date_start: savedObject.selected_date_range.start, | ||
date_end : savedObject.selected_date_range.end, | ||
date_end: savedObject.selected_date_range.end, | ||
timestamp: savedObject.selected_timestamp?.name || '', | ||
fields: savedObject.selected_fields?.tokens || [] | ||
fields: savedObject.selected_fields?.tokens || [], | ||
}, | ||
name: savedObject.name || '', | ||
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 name shouldn't have 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'll remove that |
||
description: savedObject.description || '', | ||
|
||
type: queryType, | ||
}; | ||
}); | ||
|
||
|
||
const totalItemCount = queries.length; | ||
|
||
const search = { | ||
box: { | ||
incremental: true, | ||
}, | ||
filters: [ | ||
{ | ||
type: 'field_value_selection', | ||
field: 'type', | ||
name: 'Type', | ||
multiSelect: false, | ||
options: FILTER_OPTIONS.map((i) => ({ | ||
value: i, | ||
name: i, | ||
view: `${i}`, | ||
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. Why is the 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. Yeah it can be like above lines too |
||
})), | ||
}, | ||
], | ||
}; | ||
|
||
const pagination = { | ||
pageIndex, | ||
pageSize, | ||
|
@@ -88,14 +144,13 @@ export function Table(options: TableData) { | |
return ( | ||
<div> | ||
<EuiSpacer size="xl" /> | ||
<EuiBasicTable | ||
items={queries.slice(pageIndex * pageSize, pageIndex * pageSize + pageSize)} | ||
<EuiInMemoryTable | ||
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. using InMemoryTable requires storing all items in memory instead of a constant 10 items (which is 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'm not sure about that part, i'll have a talk with Eric regarding this |
||
items={queries} | ||
columns={columns} | ||
pagination={pagination} | ||
onChange={onTableChange} | ||
search={search} | ||
/> | ||
</div> | ||
); | ||
} | ||
|
||
|
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.
We should add name as
type
here so that it's visible on the table.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.
Ani wanted the icon column name to be empty and there's a separate column named type too for the type of the query