Skip to content
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

Jump to bottom

refactor: Bootstrap to AntD - Form - iteration 3 #14502

Merged
merged 1 commit into from May 8, 2021
Merged

refactor: Bootstrap to AntD - Form - iteration 3 #14502

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions superset-frontend/spec/javascripts/explore/components/AdhocMetricEditPopover_spec.jsx
View file
Open in desktop
Expand Up @@ -20,7 +20,7 @@
import React from 'react';
import sinon from 'sinon';
import { shallow } from 'enzyme';
import { FormGroup } from 'react-bootstrap';
import { FormItem } from 'src/components/Form';
import Button from 'src/components/Button';

import { AGGREGATES } from 'src/explore/constants';
Expand Down Expand Up @@ -67,7 +67,7 @@ function setup(overrides) {
describe('AdhocMetricEditPopover', () => {
it('renders a popover with edit metric form contents', () => {
const { wrapper } = setup();
expect(wrapper.find(FormGroup)).toHaveLength(4);
expect(wrapper.find(FormItem)).toHaveLength(3);
expect(wrapper.find(Button)).toHaveLength(2);
});

Expand Down
12 changes: 6 additions & 6 deletions superset-frontend/spec/javascripts/explore/components/BoundsControl_spec.jsx
View file
Open in desktop
Expand Up @@ -17,10 +17,10 @@
* under the License.
*/
import React from 'react';
import { FormControl } from 'react-bootstrap';
import sinon from 'sinon';
import { styledMount as mount } from 'spec/helpers/theming';
import BoundsControl from 'src/explore/components/controls/BoundsControl';
import { Input } from 'src/common/components';

const defaultProps = {
name: 'y_axis_bounds',
Expand All @@ -35,13 +35,13 @@ describe('BoundsControl', () => {
wrapper = mount(<BoundsControl {...defaultProps} />);
});

it('renders two FormControls', () => {
expect(wrapper.find(FormControl)).toHaveLength(2);
it('renders two Input', () => {
expect(wrapper.find(Input)).toHaveLength(2);
});

it('errors on non-numeric', () => {
wrapper
.find(FormControl)
.find(Input)
.first()
.simulate('change', { target: { value: 's' } });
expect(defaultProps.onChange.calledWith([null, null])).toBe(true);
Expand All @@ -51,11 +51,11 @@ describe('BoundsControl', () => {
});
it('casts to numeric', () => {
wrapper
.find(FormControl)
.find(Input)
.first()
.simulate('change', { target: { value: '1' } });
wrapper
.find(FormControl)
.find(Input)
.last()
.simulate('change', { target: { value: '5' } });
expect(defaultProps.onChange.calledWith([1, 5])).toBe(true);
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/spec/javascripts/sqllab/SaveQuery_spec.jsx
View file
Open in desktop
Expand Up @@ -17,12 +17,12 @@
* under the License.
*/
import React from 'react';
import { FormControl } from 'react-bootstrap';
import { shallow } from 'enzyme';
import * as sinon from 'sinon';
import SaveQuery from 'src/SqlLab/components/SaveQuery';
import Modal from 'src/components/Modal';
import Button from 'src/components/Button';
import { FormItem } from 'src/components/Form';

describe('SavedQuery', () => {
const mockedProps = {
Expand Down Expand Up @@ -52,11 +52,11 @@ describe('SavedQuery', () => {

expect(modal.find('[data-test="cancel-query"]')).toHaveLength(1);
});
it('has 2 FormControls', () => {
it('has 2 FormItem', () => {
const wrapper = shallow(<SaveQuery {...mockedProps} />);
const modal = wrapper.find(Modal);

expect(modal.find(FormControl)).toHaveLength(2);
expect(modal.find(FormItem)).toHaveLength(2);
});
// eslint-disable-next-line jest/no-disabled-tests
it.skip('has a save button if this is a new query', () => {
Expand Down
41 changes: 18 additions & 23 deletions superset-frontend/src/SqlLab/components/SaveQuery.tsx
View file
Open in desktop
Expand Up @@ -17,12 +17,10 @@
* under the License.
*/
import React, { useState } from 'react';
import { Row, Col } from 'src/common/components';
import { FormControl, FormGroup } from 'react-bootstrap';
import { Row, Col, Input, TextArea } from 'src/common/components';
import { t, supersetTheme, styled } from '@superset-ui/core';

import Button from 'src/components/Button';
import { FormLabel } from 'src/components/Form';
import { Form, FormItem } from 'src/components/Form';
import Modal from 'src/components/Modal';
import Icon from 'src/components/Icon';

Expand Down Expand Up @@ -100,40 +98,37 @@ export default function SaveQuery({
close();
};

const onLabelChange = (e: React.FormEvent<FormControl>) => {
setLabel((e.target as HTMLInputElement).value);
const onLabelChange = (e: React.ChangeEvent<HTMLInputElement>) => {
setLabel(e.target.value);
};

const onDescriptionChange = (e: React.FormEvent<FormControl>) => {
setDescription((e.target as HTMLInputElement).value);
const onDescriptionChange = (e: React.ChangeEvent<HTMLTextAreaElement>) => {
setDescription(e.target.value);
};

const toggleSave = () => {
setShowSave(!showSave);
};

const renderModalBody = () => (
<FormGroup bsSize="small">
<Form layout="vertical">
<Row>
<Col xs={24}>
<small>
<FormLabel htmlFor="embed-height">{t('Name')}</FormLabel>
</small>
<FormControl type="text" value={label} onChange={onLabelChange} />
<FormItem label={t('Name')}>
<Input type="text" value={label} onChange={onLabelChange} />
</FormItem>
</Col>
</Row>
<br />
<Row>
<Col xs={24}>
<small>
<FormLabel htmlFor="embed-height">{t('Description')}</FormLabel>
</small>
<FormControl
rows={5}
componentClass="textarea"
value={description}
onChange={onDescriptionChange}
/>
<FormItem label={t('Description')}>
<TextArea
rows={4}
value={description}
onChange={onDescriptionChange}
/>
</FormItem>
</Col>
</Row>
{saveQueryWarning && (
Expand All @@ -149,7 +144,7 @@ export default function SaveQuery({
</div>
</>
)}
</FormGroup>
</Form>
);

return (
Expand Down
61 changes: 35 additions & 26 deletions superset-frontend/src/SqlLab/components/ScheduleQueryButton.tsx
View file
Open in desktop
Expand Up @@ -17,13 +17,12 @@
* under the License.
*/
import React, { FunctionComponent, useState } from 'react';
import Form, { FormProps, FormValidation } from 'react-jsonschema-form';
import { Row, Col } from 'src/common/components';
import { FormControl, FormGroup } from 'react-bootstrap';
import SchemaForm, { FormProps, FormValidation } from 'react-jsonschema-form';
import { Row, Col, Input, TextArea } from 'src/common/components';
import { t, styled } from '@superset-ui/core';
import * as chrono from 'chrono-node';
import ModalTrigger from 'src/components/ModalTrigger';
import { FormLabel } from 'src/components/Form';
import { Form, FormItem } from 'src/components/Form';
import './ScheduleQueryButton.less';
import Button from 'src/components/Button';

Expand Down Expand Up @@ -139,42 +138,52 @@ const ScheduleQueryButton: FunctionComponent<ScheduleQueryButtonProps> = ({
};

const renderModalBody = () => (
<FormGroup>
<Form layout="vertical">
<StyledRow>
<Col xs={24}>
<FormLabel className="control-label" htmlFor="embed-height">
{t('Label')}
</FormLabel>
<FormControl
type="text"
placeholder={t('Label for your query')}
value={label}
onChange={(event: any) => setLabel(event.target?.value)}
/>
<FormItem label={t('Label')}>
<Input
type="text"
placeholder={t('Label for your query')}
value={label}
onChange={(event: React.ChangeEvent<HTMLInputElement>) =>
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move away from having to set the values in the component state. The beauty of the Antd forms is that they can handle pretty much everything in the background. We could just set a name in the FormItem and pass the initialValues to the Form as an object. All the rest will be handled internally by Antd. This looks like the perfect case as there is no custom manipulation/side effects of the data. Other forms might gain the same benefit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@geido I totally agree with you. I noticed that all screens were using forms just to reuse the layout spacing, but none were using validation, state management, etc. This problem is happening everywhere there is a form. The way I'm tackling this is to first get rid of Boostrap and try to avoid adding more refactoring to the components. After that phase, we can revisit all the form components focusing specifically on using the form benefits. This needs a discussion because we need to decide if we're going to use form validation.

@rusackas

setLabel(event.target.value)
}
/>
</FormItem>
</Col>
</StyledRow>
<StyledRow>
<Col xs={24}>
<FormLabel className="control-label" htmlFor="embed-height">
{t('Description')}
</FormLabel>
<FormControl
componentClass="textarea"
placeholder={t('Write a description for your query')}
value={description}
onChange={(event: any) => setDescription(event.target?.value)}
/>
<FormItem label={t('Description')}>
<TextArea
rows={4}
placeholder={t('Write a description for your query')}
value={description}
onChange={(event: React.ChangeEvent<HTMLTextAreaElement>) =>
setDescription(event.target.value)
}
/>
</FormItem>
</Col>
</StyledRow>
<Row>
<Col xs={24}>
<div className="json-schema">
<Form
<SchemaForm
schema={getJSONSchema()}
uiSchema={getUISchema}
onSubmit={onScheduleSubmit}
validate={getValidator()}
/>
>
<Button
buttonStyle="primary"
htmlType="submit"
css={{ float: 'right' }}
>
Submit
</Button>
</SchemaForm>
</div>
</Col>
</Row>
Expand All @@ -185,7 +194,7 @@ const ScheduleQueryButton: FunctionComponent<ScheduleQueryButtonProps> = ({
</Col>
</Row>
)}
</FormGroup>
</Form>
);

return (
Expand Down
12 changes: 7 additions & 5 deletions superset-frontend/src/common/components/index.tsx
View file
Open in desktop
Expand Up @@ -89,11 +89,13 @@ export const Menu = Object.assign(AntdMenu, {
});

export const Input = styled(AntdInput)`
&[type='text'],
&[type='textarea'] {
border: 1px solid ${({ theme }) => theme.colors.secondary.light3};
border-radius: ${({ theme }) => theme.borderRadius}px;
}
border: 1px solid ${({ theme }) => theme.colors.secondary.light3};
border-radius: ${({ theme }) => theme.borderRadius}px;
`;

export const TextArea = styled(AntdInput.TextArea)`
border: 1px solid ${({ theme }) => theme.colors.secondary.light3};
border-radius: ${({ theme }) => theme.borderRadius}px;
`;

export const NoAnimationDropdown = (props: DropDownProps) => (
Expand Down
33 changes: 19 additions & 14 deletions superset-frontend/src/components/Form/FormItem.tsx
View file
Open in desktop
Expand Up @@ -21,23 +21,28 @@ import Form, { FormItemProps } from 'antd/lib/form';
import { styled } from '@superset-ui/core';

const StyledItem = styled(Form.Item)`
.ant-form-item-label > label {
text-transform: uppercase;
font-size: ${({ theme }) => theme.typography.sizes.s}px;
color: ${({ theme }) => theme.colors.grayscale.base};
${({ theme }) => `
.ant-form-item-label {
padding-bottom: ${theme.gridUnit}px;
& > label {
text-transform: uppercase;
font-size: ${theme.typography.sizes.s}px;
color: ${theme.colors.grayscale.base};

&.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
&::before {
display: none;
}
&::after {
display: inline-block;
color: ${({ theme }) => theme.colors.error.base};
font-size: ${({ theme }) => theme.typography.sizes.m}px;
content: '*';
&.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
&::before {
display: none;
}
&::after {
display: inline-block;
color: ${theme.colors.error.base};
font-size: ${theme.typography.sizes.m}px;
content: '*';
}
}
}
}
}
`}
`;

export default function FormItem(props: FormItemProps) {
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/dashboard/components/PropertiesModal/index.jsx
View file
Open in desktop
Expand Up @@ -350,7 +350,7 @@ class PropertiesModal extends React.PureComponent {
<ColorSchemeControlWrapper
onChange={this.onColorSchemeChange}
colorScheme={values.colorScheme}
labelMargin={8}
labelMargin={4}
/>
</Col>
</Row>
Expand Down Expand Up @@ -413,7 +413,7 @@ class PropertiesModal extends React.PureComponent {
<ColorSchemeControlWrapper
onChange={this.onColorSchemeChange}
colorScheme={values.colorScheme}
labelMargin={8}
labelMargin={4}
/>
</Col>
</Row>
Expand Down
6 changes: 2 additions & 4 deletions superset-frontend/src/explore/components/DataTableControl/index.tsx
View file
Open in desktop
Expand Up @@ -18,10 +18,9 @@
*/
import React, { useMemo } from 'react';
import { styled, t } from '@superset-ui/core';
import { FormControl } from 'react-bootstrap';
import { Column } from 'react-table';
import debounce from 'lodash/debounce';

import { Input } from 'src/common/components';
import {
BOOL_FALSE_DISPLAY,
BOOL_TRUE_DISPLAY,
Expand Down Expand Up @@ -73,9 +72,8 @@ export const FilterInput = ({
}) => {
const debouncedChangeHandler = debounce(onChangeHandler, SLOW_DEBOUNCE);
return (
<FormControl
<Input
placeholder={t('Search')}
bsSize="sm"
onChange={(event: any) => {
const filterText = event.target.value;
debouncedChangeHandler(filterText);
Expand Down