-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
[explore v2] populate dynamic select field options #1543
Conversation
@@ -11,7 +11,8 @@ const defaultProps = { | |||
fieldOverrides: {}, | |||
}; | |||
|
|||
function getFieldData(fs, fieldOverrides) { | |||
function getFieldData(fs, fieldOverrides, fields) { | |||
console.log('fields', fields) |
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.
should we delete this?
@mistercrunch @vera-liu PTAL, this is ready for review now. |
} | ||
}); | ||
} else { | ||
// Clear all Select options | ||
dispatch(clearAllOpts()); | ||
// in what case don't we have a datasource id? |
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.
in react-select there's an option to empty the select field, namely datasource could be null, not sure if this is needed
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'm not sure it's needed either. in the explore view we should always have a datasource. we can remove in a follow up PR.
} | ||
|
||
export function setColumnOpts(columnOpts) { | ||
return { type: SET_COLUMN_OPTS, columnOpts }; | ||
export function setDatasourceType(datasourceType) { |
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 used to think this would be used for setting Druid/Table for datasource, but it seems that we don't have the option to switch datasource type in explore view? @mistercrunch
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 don't think we actually need this. we can change a datasource but there is not a concept of changing the datasource type only. datasource type is tied to the datasource. i think this shows up like this because of some white space diffing. we can remove in a follow up pr.
… dynamic field choices
@@ -1655,16 +1658,11 @@ export const initialState = { | |||
metricsOpts: [], | |||
columnOpts: [], | |||
orderingOpts: [], | |||
searchBox: false, |
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 think we could delete 1661-1664 here since we now have them in form_data. I deleted it in a previous PR but somehow it must be accidentally restored during the rebase.
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.
ah ok will do. thx!
@@ -1655,16 +1658,11 @@ export const initialState = { | |||
metricsOpts: [], |
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.
These Opts could be deleted as well.
looks good! |
fetch_datasource_metadata
/fetch_datasource_metadata/
returnstodo: