schizophrenicmaniac commented on code in PR #44963:
URL: https://github.com/apache/superset/pull/44963#discussion_r4205254962
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -580,14 +580,21 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
const controlOrCommand = event.ctrlKey || event.metaKey;
if (controlOrCommand) {
const isEnter = event.key === 'Enter' || event.keyCode === 13;
- if (isEnter) {
+ // Match the Run button, which is disabled while any control has
+ // validation errors and swapped for Stop while the chart is loading
+ const hasValidationErrors = Object.values(props.controls).some(
+ control =>
+ control.validationErrors && control.validationErrors.length > 0,
+ );
Review Comment:
Moved the check into a `hasControlErrors` memo that the keydown handler and
the mount effect now share. I left `errorMessage` and `dataTabErrorMessage`
alone because they need the actual list of controls with errors (and the Data
tab one skips matrixify controls), so a boolean helper doesn't really fit there.
##########
superset-frontend/src/explore/components/ExploreViewContainer/ExploreViewContainer.test.tsx:
##########
@@ -854,6 +855,74 @@ test('shows error indicator with function labels', async
() => {
expect(await screen.findByText(/Metric is required/)).toBeInTheDocument();
});
+const renderAfterInitialQuery = async (initialState: object) => {
+ const triggerQuerySpy = jest.spyOn(chartActions, 'triggerQuery');
+ const store = createStore(initialState, reducerIndex);
+ renderWithRouter({ initialState, store: store as Store });
+ // the mocked chart panel queries on mount; finish it like a real chart would
+ await waitFor(() => expect(triggerQuerySpy).toHaveBeenCalled());
+ act(() => {
+ store.dispatch(chartActions.chartUpdateSucceeded([], 1));
+ });
+ triggerQuerySpy.mockClear();
+ return { store, triggerQuerySpy };
+};
+
+test('Ctrl+Enter runs the query when controls are valid', async () => {
+ const { triggerQuerySpy } = await renderAfterInitialQuery(reduxState);
+
+ await userEvent.keyboard('{Control>}{Enter}{/Control}');
+
+ expect(triggerQuerySpy).toHaveBeenCalledWith(true, 1);
+});
+
+test('Cmd+Enter runs the query when controls are valid', async () => {
+ const { triggerQuerySpy } = await renderAfterInitialQuery(reduxState);
+
+ await userEvent.keyboard('{Meta>}{Enter}{/Meta}');
+
+ expect(triggerQuerySpy).toHaveBeenCalledWith(true, 1);
+});
+
+test('Ctrl+Enter does not run the query when controls have validation errors',
async () => {
+ const { triggerQuerySpy } = await renderAfterInitialQuery({
+ ...reduxState,
+ explore: {
+ ...reduxState.explore,
+ controls: {
+ ...reduxState.explore.controls,
+ metric: {
+ value: '',
+ label: 'Metric',
+ validationErrors: ['Metric is required'],
+ },
+ },
Review Comment:
`reduxState` doesn't have a `metric` control, so there's nothing to spread
here. The other validation error tests in this file build it the same way, so I
kept it consistent with those.
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -580,14 +580,21 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
const controlOrCommand = event.ctrlKey || event.metaKey;
if (controlOrCommand) {
const isEnter = event.key === 'Enter' || event.keyCode === 13;
- if (isEnter) {
+ // Match the Run button, which is disabled while any control has
+ // validation errors and swapped for Stop while the chart is loading
+ const hasValidationErrors = Object.values(props.controls).some(
+ control =>
+ control.validationErrors && control.validationErrors.length > 0,
+ );
+ const isLoading = props.chart.chartStatus === 'loading';
+ if (isEnter && !hasValidationErrors && !isLoading) {
onQuery();
}
Review Comment:
The handler now depends on two booleans (`hasControlErrors` and
`isChartLoading`) instead of `props.controls`. It still gets recreated when
controls change, since `onQuery` depends on `props.controls` and was already in
the deps before this PR. That's needed anyway so the shortcut queries with the
current controls.
##########
superset-frontend/src/explore/components/ExploreViewContainer/index.tsx:
##########
@@ -580,14 +580,21 @@ function ExploreViewContainer(props:
ExploreViewContainerProps) {
const controlOrCommand = event.ctrlKey || event.metaKey;
if (controlOrCommand) {
const isEnter = event.key === 'Enter' || event.keyCode === 13;
- if (isEnter) {
+ // Match the Run button, which is disabled while any control has
+ // validation errors and swapped for Stop while the chart is loading
+ const hasValidationErrors = Object.values(props.controls).some(
+ control =>
+ control.validationErrors && control.validationErrors.length > 0,
+ );
+ const isLoading = props.chart.chartStatus === 'loading';
+ if (isEnter && !hasValidationErrors && !isLoading) {
onQuery();
}
// Note: Ctrl+S save functionality removed due to type
incompatibilities
// between Slice types. Use the save button instead.
}
},
- [onQuery],
+ [onQuery, props.controls, props.chart.chartStatus],
Review Comment:
Same change as the comment above: the deps are now `[onQuery,
hasControlErrors, isChartLoading]`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
