-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
fix: Explore "Change Dataset" UX Enhancements #12006
Conversation
…into hugh/so-1119-virtual-ds
…into hugh/so-1119-virtual-ds
…into hugh/so-1119-virtual-ds
…into hugh/so-1119-virtual-ds
Codecov Report
@@ Coverage Diff @@
## master #12006 +/- ##
==========================================
+ Coverage 67.71% 67.73% +0.02%
==========================================
Files 952 952
Lines 46686 46703 +17
Branches 4577 4578 +1
==========================================
+ Hits 31614 31636 +22
+ Misses 14959 14955 -4
+ Partials 113 112 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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, I just left a few comments on nits.
import { getClientErrorObject } from '../utils/getClientErrorObject'; | ||
import Loading from '../components/Loading'; | ||
import withToasts from '../messageToasts/enhancers/withToasts'; | ||
|
||
const CONFIRM_WARNING_MESSAGE = t( | ||
'Warning! Changing the dataset may break the chart if the metadata does not exist in the target dataset', |
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 what this message means, can we clarify what metadata is necessary?
const CHANGE_WARNING_MSG = t( | ||
'Changing the dataset may break the chart if the chart relies ' + | ||
'on columns or metadata that does not exist in the target dataset', | ||
); | ||
|
||
const useDebouncedEffect = (effect: any, delay: number) => { |
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.
Nit: let's move this to some utils.ts
module, so it can be used by other components.
} = useListViewResource<Dataset>('dataset', t('dataset'), addDangerToast); | ||
|
||
const selectDatasource = useCallback( | ||
(datasource: { type: string; id: number; uid: string }) => { |
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.
Nit: let's make this a type, so that you don't have to declare it as <any>
when creating the state in line 100.
await fetchData({ | ||
pageIndex: 0, | ||
pageSize: 20, | ||
filters: [], | ||
sortBy: [{ id: 'changed_on_delta_humanized' }], | ||
}); |
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.
Nit: you might want to move this to a const, eg:
const emptyRequest = {
pageIndex: 0,
pageSize: 20,
filters: [],
sortBy: [{ id: 'change_on_delta_humanized' }],
};
Then in your useDebounceEffect
you can reuse it too:
fetchData({
...emptyRequest,
filters: [{ id: 'table_name', 'operator': 'ct', value: 'filter' }],
});
/> | ||
{!confirmChange && ( | ||
<> | ||
<Alert bsStyle="warning"> |
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 like the warning message here, I should use it in the overwrite confirmation dialog!
.btn-container { | ||
display: flex; | ||
justify-content: flex-end; | ||
padding: 0px 15px; |
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.
might be able to use some of the theme gridunits here
…into hugh/so-1119-virtual-ds-modal-refactor
SUMMARY
On changing dataset in explore view, the tableview will include both virtual and physical tables, and will prompt users to make sure they are okay we with changing dataset for the explore view.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
Change Datasets
Proceed
on the modalADDITIONAL INFORMATION