Skip to content

fix(AlertReportModal): Text Area Change #17176

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.

Sign up for GitHub

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

Merged
eschutho merged 5 commits into from
Oct 29, 2021
Merged

fix(AlertReportModal): Text Area Change #17176

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2a09999
Text Area Change
AAfghahi Oct 20, 2021
973cfda
added unique key to component
AAfghahi Oct 26, 2021
294ce97
tests passing
AAfghahi Oct 26, 2021
145fac2
changed css
AAfghahi Oct 27, 2021
5bab212
data flow and naming convention
AAfghahi Oct 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion superset-frontend/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion superset-frontend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@
"query-string": "^6.13.7",
"re-resizable": "^6.6.1",
"react": "^16.13.1",
"react-ace": "^5.10.0",
"react-ace": "^9.4.4",
"react-checkbox-tree": "^1.5.1",
"react-color": "^2.13.8",
"react-datetime": "^3.0.4",
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/SqlLab/components/TemplateParamsEditor/TemplateParamsEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('TemplateParamsEditor', () => {
const spy = jest.spyOn(brace, 'acequire');
spy.mockReturnValue({ setCompleters: () => 'foo' });
await waitFor(() => {
expect(baseElement.querySelector('#brace-editor')).toBeInTheDocument();
expect(baseElement.querySelector('#ace-editor')).toBeInTheDocument();
Comment thread
eschutho marked this conversation as resolved.
});
});
});
2 changes: 1 addition & 1 deletion superset-frontend/src/components/AsyncAceEditor/AsyncAceEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import AsyncAceEditor, {
AsyncAceEditorOptions,
} from 'src/components/AsyncAceEditor';

const selector = '[id="brace-editor"]';
const selector = '[id="ace-editor"]';

test('renders SQLEditor', async () => {
const { container } = render(<SQLEditor />);
Expand Down
8 changes: 3 additions & 5 deletions superset-frontend/src/components/AsyncAceEditor/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
Position,
TextMode as OrigTextMode,
} from 'brace';
import AceEditor, { AceEditorProps } from 'react-ace';
import AceEditor, { IAceEditorProps } from 'react-ace';
import AsyncEsmComponent, {
PlaceholderProps,
} from 'src/components/AsyncEsmComponent';
Expand Down Expand Up @@ -72,7 +72,7 @@ const aceModuleLoaders = {

export type AceModule = keyof typeof aceModuleLoaders;

export type AsyncAceEditorProps = AceEditorProps & {
export type AsyncAceEditorProps = IAceEditorProps & {
keywords?: AceCompleterKeyword[];
};

Expand All @@ -83,7 +83,7 @@ export type AsyncAceEditorOptions = {
defaultTheme?: AceEditorTheme;
defaultTabSize?: number;
placeholder?: React.ComponentType<
PlaceholderProps & Partial<AceEditorProps>
PlaceholderProps & Partial<IAceEditorProps>
> | null;
};

Expand Down Expand Up @@ -120,7 +120,6 @@ export default function AsyncAceEditor(
theme = inferredTheme,
tabSize = defaultTabSize,
defaultValue = '',
value = '',
...props
},
ref,
Expand Down Expand Up @@ -153,7 +152,6 @@ export default function AsyncAceEditor(
theme={theme}
tabSize={tabSize}
defaultValue={defaultValue}
value={value || ''}

@eschutho eschutho Oct 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug that I fixed on sql lab where if you passed in value as null (not undefined), sql lab would crash. Can you test to see if this was fixed with the new version of aceeditor?

{...props}
/>
);
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/dashboard/components/CssEditor/CssEditor.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
import React from 'react';
import { render, screen, waitFor } from 'spec/helpers/testing-library';
import { CssEditor as AceCssEditor } from 'src/components/AsyncAceEditor';
import { AceEditorProps } from 'react-ace';
import { IAceEditorProps } from 'react-ace';
import userEvent from '@testing-library/user-event';
import CssEditor from '.';

jest.mock('src/components/AsyncAceEditor', () => ({
CssEditor: ({ value, onChange }: AceEditorProps) => (
CssEditor: ({ value, onChange }: IAceEditorProps) => (
<textarea
defaultValue={value}
onChange={value => onChange?.(value.target.value)}
Expand Down
30 changes: 6 additions & 24 deletions superset-frontend/src/explore/components/controls/TextAreaControl.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@
import React from 'react';
import PropTypes from 'prop-types';
import { TextArea } from 'src/common/components';
import { debounce } from 'lodash';
import { t } from '@superset-ui/core';

import { FAST_DEBOUNCE } from 'src/constants';
import Button from 'src/components/Button';
import { TextAreaEditor } from 'src/components/AsyncAceEditor';
import ModalTrigger from 'src/components/ModalTrigger';
Expand All @@ -32,7 +30,7 @@ import ControlHeader from 'src/explore/components/ControlHeader';
const propTypes = {
name: PropTypes.string,
onChange: PropTypes.func,
value: PropTypes.string,
defaultValue: PropTypes.string,
height: PropTypes.number,
minLines: PropTypes.number,
maxLines: PropTypes.number,
Expand All @@ -51,7 +49,7 @@ const propTypes = {

const defaultProps = {
onChange: () => {},
value: '',
defaultValue: '',
height: 250,
minLines: 3,
maxLines: 10,
Expand All @@ -60,29 +58,12 @@ const defaultProps = {
};

export default class TextAreaControl extends React.Component {
constructor() {
super();
this.state = {
value: '',
};
this.onAceChangeDebounce = debounce(value => {
this.onAceChange(value);
}, FAST_DEBOUNCE);
}

onControlChange(event) {
const { value } = event.target;
this.setState({ value });
this.props.onChange(value);
}

onAceChange(value) {
this.setState({ value });
this.props.onChange(value);
}

renderEditor(inModal = false) {
const value = this.state.value || this.props.value;
const minLines = inModal ? 40 : this.props.minLines || 12;
if (this.props.language) {
const style = { border: '1px solid #CCC' };
Expand All @@ -95,20 +76,21 @@ export default class TextAreaControl extends React.Component {
style={style}
minLines={minLines}
maxLines={inModal ? 1000 : this.props.maxLines}
onChange={this.onAceChangeDebounce}
onChange={this.props.onChange}
width="100%"
height={`${minLines}em`}
editorProps={{ $blockScrolling: true }}
value={value}
defaultValue={this.props.defaultValue}
readOnly={this.props.readOnly}
key={this.props.name}
/>
);
}
return (
<TextArea
placeholder={t('textarea')}
onChange={this.onControlChange.bind(this)}
value={value}
value={this.props.defaultValue}

@eschutho eschutho Oct 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks like it will never change value if you type in it?

@AAfghahi AAfghahi Oct 27, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mentioned this in our pair programming as a potential issue so I tested it. I removed language from where we are invoking it in the AlertReportModal and got this
Screen Shot 2021-10-27 at 11 11 15 AM

disabled={this.props.readOnly}
style={{ height: this.props.height }}
/>
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/alert/AlertReportModal.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ describe('AlertReportModal', () => {
const editWrapper = await mountAndWait(props);
const input = editWrapper.find(TextAreaControl);
expect(input).toExist();
expect(input.props().value).toEqual('SELECT NaN');
expect(input.props().defaultValue).toEqual('SELECT NaN');
});

it('renders five select element when in report mode', () => {
Expand Down
5 changes: 4 additions & 1 deletion superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,8 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
NotificationSetting[]
>([]);

const generateKey = () => `${new Date().getTime()}`;

const onNotificationAdd = () => {
const settings: NotificationSetting[] = notificationSettings.slice();

Expand Down Expand Up @@ -1136,7 +1138,8 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
maxLines={15}
onChange={onSQLChange}
readOnly={false}
value={currentAlert ? currentAlert.sql : ''}
defaultValue={currentAlert?.sql}

@eschutho eschutho Oct 27, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to pass in the resource.sql here. Otherwise you're passing in the latest state and not just the api response. This is likely why the TextArea component is continuing to update, too.
Also small nit, looking at this again, I would recommend calling this initialValue instead of defaultValue; It's not a default, because it's not static and not a fallback if data isn't given, if that makes sense.

@AAfghahi AAfghahi Oct 28, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, that makes sense, though I created a new value prop that is passing in currentAlert?.sql in order to pass it in as value for the TextArea (which is the version that shows up when you don't designate a language).

@AAfghahi AAfghahi Oct 28, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my thought process was that since TextArea, unlike TextAreaControl, is a controlled component that we would need a changing value, which currentAlert.sql is. Looking through the code, there is currently no place where we do use TextArea, we always pass in a language. But I wanted to not accidentally mess something up.

key={currentAlert?.id || generateKey()}

@eschutho eschutho Oct 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this generate a new key each time it rerenders?

@AAfghahi AAfghahi Oct 27, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, and I actually moved it over to the AlertReportModal because of this. Earlier I had done this and the state issue had still persisted, but when I test it now it is working fine. So I must have originally tested it during a bad iteration.

/>
</StyledInputContainer>
<div className="inline-container wrap">
Expand Down