You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/01/30 00:05:00 UTC
[GitHub] [incubator-superset] suddjian opened a new pull request #9051:
[charts
suddjian opened a new pull request #9051: [charts
URL: https://github.com/apache/incubator-superset/pull/9051
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373603978
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
+ />
+ <p className="help-block">
+ {t(
+ 'The description can be displayed as widget headers in the dashboard view. Supports markdown.',
+ )}
+ </p>
+ </FormGroup>
+ </Col>
+ <Col md={6}>
+ <h3>{t('Configuration')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="cacheTimeout">
+ {t('Cache Timeout')}
+ </label>
+ <FormControl
+ name="cacheTimeout"
+ type="text"
+ bsSize="sm"
+ value={cacheTimeout}
+ onChange={event =>
+ setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
Review comment:
The marshmallow schema on the endpoint wants an integer
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373645587
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
+ />
+ <p className="help-block">
+ {t(
+ 'The description can be displayed as widget headers in the dashboard view. Supports markdown.',
+ )}
+ </p>
+ </FormGroup>
+ </Col>
+ <Col md={6}>
+ <h3>{t('Configuration')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="cacheTimeout">
+ {t('Cache Timeout')}
+ </label>
+ <FormControl
+ name="cacheTimeout"
+ type="text"
+ bsSize="sm"
+ value={cacheTimeout}
+ onChange={event =>
+ setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
Review comment:
We could use `type="number` alongside the regex filter to get the extra little browser widgets that come with number inputs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373594565
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
+ />
+ <p className="help-block">
+ {t(
+ 'The description can be displayed as widget headers in the dashboard view. Supports markdown.',
+ )}
+ </p>
+ </FormGroup>
+ </Col>
+ <Col md={6}>
+ <h3>{t('Configuration')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="cacheTimeout">
+ {t('Cache Timeout')}
+ </label>
+ <FormControl
+ name="cacheTimeout"
+ type="text"
+ bsSize="sm"
+ value={cacheTimeout}
+ onChange={event =>
+ setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
Review comment:
That's what I did at first, but I wasn't sure if we wanted to allow non-integers. Do you have any insight into whether that would be ok?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r374252657
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,239 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+ const [ownerOptions, setOwnerOptions] = useState(null);
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(
+ slice.cache_timeout != null ? slice.cache_timeout : '',
+ );
+ const [owners, setOwners] = useState(null);
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ async function fetchOwners() {
+ try {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ response.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (response) {
+ const clientError = await getClientErrorObject(response);
+ showError(clientError);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setOwnerOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
Review comment:
missed this before, please use complete words for variable names
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373580356
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
Review comment:
is there a reason we default to empty string here? Seems like null would make sense since that's what you're sending to the server, and then in the UI we can replace with an empty string if needed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373269982
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
Review comment:
Perhaps. If you think it's worth messing around with global styles for this specific thing I'll do it. Without that line, you can grab+drag the textarea out into the right column of the modal layout.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373580718
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
Review comment:
full word variable names please! prefer: `response`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373632831
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
+ />
+ <p className="help-block">
+ {t(
+ 'The description can be displayed as widget headers in the dashboard view. Supports markdown.',
+ )}
+ </p>
+ </FormGroup>
+ </Col>
+ <Col md={6}>
+ <h3>{t('Configuration')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="cacheTimeout">
+ {t('Cache Timeout')}
+ </label>
+ <FormControl
+ name="cacheTimeout"
+ type="text"
+ bsSize="sm"
+ value={cacheTimeout}
+ onChange={event =>
+ setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
Review comment:
I suppose that when sending the request, you could do a `parseInt` on the value to remove the decimal. Maybe that's sufficient then? At least we don't need to run the validator onChange then
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373591850
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
Review comment:
I went with empty string since this is used for the state of the `<input>`, and input values should not be null. Empty strings are converted to null when sending, to allow users to unset fields. It's cleaner this way IMO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373269025
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
Review comment:
big π
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373595134
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
Review comment:
Fair. I can see how that could get out of hand in a large component.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9051:
[explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#issuecomment-580392871
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=h1) Report
> Merging [#9051](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a267446f7b3f707d0a49648067087d9767709773?src=pr&el=desc) will **decrease** coverage by `0.28%`.
> The diff coverage is `6.06%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9051 +/- ##
==========================================
- Coverage 59.43% 59.15% -0.29%
==========================================
Files 369 370 +1
Lines 11743 11813 +70
Branches 2884 2899 +15
==========================================
+ Hits 6980 6988 +8
- Misses 4584 4646 +62
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...ts/src/explore/components/ExploreActionButtons.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvRXhwbG9yZUFjdGlvbkJ1dHRvbnMuanN4) | `100% <ΓΈ> (ΓΈ)` | :arrow_up: |
| [.../assets/src/explore/components/PropertiesModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvUHJvcGVydGllc01vZGFsLmpzeA==) | `0% <0%> (ΓΈ)` | |
| [...rset/assets/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL3JlZHVjZXJzL2V4cGxvcmVSZWR1Y2VyLmpz) | `25% <0%> (-0.93%)` | :arrow_down: |
| [...erset/assets/src/explore/actions/exploreActions.js](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2FjdGlvbnMvZXhwbG9yZUFjdGlvbnMuanM=) | `34.32% <33.33%> (-0.05%)` | :arrow_down: |
| [...sets/src/explore/components/DisplayQueryButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvRGlzcGxheVF1ZXJ5QnV0dG9uLmpzeA==) | `51.38% <37.5%> (-1.74%)` | :arrow_down: |
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `54.21% <0%> (ΓΈ)` | :arrow_up: |
| [superset/assets/src/components/TableSelector.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jb21wb25lbnRzL1RhYmxlU2VsZWN0b3IuanN4) | `78.35% <0%> (+0.32%)` | :arrow_up: |
| [...uperset/assets/src/dashboard/components/Header.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9IZWFkZXIuanN4) | `42.06% <0%> (+0.93%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=footer). Last update [a267446...240388f](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373594067
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
Review comment:
null is used while it's loading, so that the input can be disabled.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373582754
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
Review comment:
why not default to empty array instead of null?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373268969
##########
File path: superset/assets/src/explore/components/DisplayQueryButton.jsx
##########
@@ -222,6 +233,20 @@ export default class DisplayQueryButton extends React.PureComponent {
pullRight
id="query"
>
+ {this.props.slice && (
+ <>
+ <MenuItem onClick={this.openPropertiesModal}>
+ {t('Edit properties')}
+ </MenuItem>
+ <PropertiesModal
+ slice={this.props.slice}
+ show={this.state.propertiesModalOpen}
+ onHide={this.closePropertiesModal}
+ onSave={this.onSliceSave}
Review comment:
oops, yeah that shouldn't be there anymore
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] nytai commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
nytai commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373234317
##########
File path: superset/assets/src/explore/components/DisplayQueryButton.jsx
##########
@@ -222,6 +233,20 @@ export default class DisplayQueryButton extends React.PureComponent {
pullRight
id="query"
>
+ {this.props.slice && (
+ <>
+ <MenuItem onClick={this.openPropertiesModal}>
+ {t('Edit properties')}
+ </MenuItem>
+ <PropertiesModal
+ slice={this.props.slice}
+ show={this.state.propertiesModalOpen}
+ onHide={this.closePropertiesModal}
+ onSave={this.onSliceSave}
Review comment:
Is this defined anywhere? Seems to be overridden in the actual component with a redux action.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373580568
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
Review comment:
i assume this is a number, so if the cache_timeout was 0, then the state would be defaulted to empty string. probably not intended
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373592116
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
Review comment:
Good catch
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar merged pull request #9051: [explore]
Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373643017
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
+ />
+ <p className="help-block">
+ {t(
+ 'The description can be displayed as widget headers in the dashboard view. Supports markdown.',
+ )}
+ </p>
+ </FormGroup>
+ </Col>
+ <Col md={6}>
+ <h3>{t('Configuration')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="cacheTimeout">
+ {t('Cache Timeout')}
+ </label>
+ <FormControl
+ name="cacheTimeout"
+ type="text"
+ bsSize="sm"
+ value={cacheTimeout}
+ onChange={event =>
+ setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
Review comment:
`parseInt` can return `NaN` for certain inputs. An input of `type="number"` allows using "e" and decimal points. I'd prefer not to open the `NaN` door, personally.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373237896
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
Review comment:
ECMAScript proposal: `event.justStopAlready()` that does both these things.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373577788
##########
File path: superset/assets/src/explore/components/DisplayQueryButton.jsx
##########
@@ -75,9 +77,12 @@ export default class DisplayQueryButton extends React.PureComponent {
error: null,
filterText: '',
sqlSupported: datasource && datasource.split('__')[1] === 'table',
+ propertiesModalOpen: false,
Review comment:
small nit: i generally prefer to start boolean attributes with `is` or `has`, so this would be `isPropertiesModalOpen`. i find it adds a bit of clarity
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373239620
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
Review comment:
If these were defined above with `useState(slice.cache_timeout || null)` instead of `useState(slice.cache_timeout || '')` could you just use it at face value here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373582568
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
+ />
+ <p className="help-block">
+ {t(
+ 'The description can be displayed as widget headers in the dashboard view. Supports markdown.',
+ )}
+ </p>
+ </FormGroup>
+ </Col>
+ <Col md={6}>
+ <h3>{t('Configuration')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="cacheTimeout">
+ {t('Cache Timeout')}
+ </label>
+ <FormControl
+ name="cacheTimeout"
+ type="text"
+ bsSize="sm"
+ value={cacheTimeout}
+ onChange={event =>
+ setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
Review comment:
I think if you made the `FormControl`'s input type `"number"` then you could get rid of this logic
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373269200
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
Review comment:
nope, the user might want to unset the cache timeout
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373579603
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
Review comment:
a bit of a nit, but i find it easier to define all the hooks at the top of the function, then put all the inline functions/logic below. it's probably not necessary in this component since it's pretty simple, but it prevents you from making a mistake like putting a hook inside conditionally executing code
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373645587
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
+ />
+ <p className="help-block">
+ {t(
+ 'The description can be displayed as widget headers in the dashboard view. Supports markdown.',
+ )}
+ </p>
+ </FormGroup>
+ </Col>
+ <Col md={6}>
+ <h3>{t('Configuration')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="cacheTimeout">
+ {t('Cache Timeout')}
+ </label>
+ <FormControl
+ name="cacheTimeout"
+ type="text"
+ bsSize="sm"
+ value={cacheTimeout}
+ onChange={event =>
+ setCacheTimeout(event.target.value.replace(/[^0-9]/, ''))
Review comment:
We could use `type="number` alongside the regex filter to get the extra little browser widgets that come with number inputs if you think that'd be valuable, but I think the regex does a job here that isn't offered by html.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] suddjian commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r374294927
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,239 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+ const [ownerOptions, setOwnerOptions] = useState(null);
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(
+ slice.cache_timeout != null ? slice.cache_timeout : '',
+ );
+ const [owners, setOwners] = useState(null);
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ async function fetchOwners() {
+ try {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ response.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (response) {
+ const clientError = await getClientErrorObject(response);
+ showError(clientError);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setOwnerOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
Review comment:
:+1: Thanks for the review!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9051: [explore]
Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#issuecomment-580392871
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=h1) Report
> Merging [#9051](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/a267446f7b3f707d0a49648067087d9767709773?src=pr&el=desc) will **decrease** coverage by `0.28%`.
> The diff coverage is `5.97%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9051 +/- ##
==========================================
- Coverage 59.43% 59.15% -0.29%
==========================================
Files 369 370 +1
Lines 11743 11814 +71
Branches 2884 2901 +17
==========================================
+ Hits 6980 6988 +8
- Misses 4584 4647 +63
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=tree) | Coverage Ξ | |
|---|---|---|
| [...ts/src/explore/components/ExploreActionButtons.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvRXhwbG9yZUFjdGlvbkJ1dHRvbnMuanN4) | `100% <ΓΈ> (ΓΈ)` | :arrow_up: |
| [.../assets/src/explore/components/PropertiesModal.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvUHJvcGVydGllc01vZGFsLmpzeA==) | `0% <0%> (ΓΈ)` | |
| [...rset/assets/src/explore/reducers/exploreReducer.js](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL3JlZHVjZXJzL2V4cGxvcmVSZWR1Y2VyLmpz) | `25% <0%> (-0.93%)` | :arrow_down: |
| [...erset/assets/src/explore/actions/exploreActions.js](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2FjdGlvbnMvZXhwbG9yZUFjdGlvbnMuanM=) | `34.32% <33.33%> (-0.05%)` | :arrow_down: |
| [...sets/src/explore/components/DisplayQueryButton.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9leHBsb3JlL2NvbXBvbmVudHMvRGlzcGxheVF1ZXJ5QnV0dG9uLmpzeA==) | `51.38% <37.5%> (-1.74%)` | :arrow_down: |
| [superset/assets/src/components/TableSelector.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9jb21wb25lbnRzL1RhYmxlU2VsZWN0b3IuanN4) | `78.35% <0%> (+0.32%)` | :arrow_up: |
| [...uperset/assets/src/dashboard/components/Header.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9051/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9kYXNoYm9hcmQvY29tcG9uZW50cy9IZWFkZXIuanN4) | `42.06% <0%> (+0.93%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=footer). Last update [a267446...183d4f2](https://codecov.io/gh/apache/incubator-superset/pull/9051?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373581110
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
Review comment:
why not just `error`? Or if you want to be specific, `clientError`. It's pretty obvious it's an object from the function name, and once we add typescript, that'll be enforced anyway
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] rusackas commented on a change in pull request
#9051: [explore] Modal to edit chart properties
Posted by GitBox <gi...@apache.org>.
rusackas commented on a change in pull request #9051: [explore] Modal to edit chart properties
URL: https://github.com/apache/incubator-superset/pull/9051#discussion_r373241584
##########
File path: superset/assets/src/explore/components/PropertiesModal.jsx
##########
@@ -0,0 +1,241 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useState, useEffect, useRef } from 'react';
+import { connect } from 'react-redux';
+import { bindActionCreators } from 'redux';
+import {
+ Button,
+ Modal,
+ Row,
+ Col,
+ FormControl,
+ FormGroup,
+} from 'react-bootstrap';
+import Dialog from 'react-bootstrap-dialog';
+import Select from 'react-select';
+import { t } from '@superset-ui/translation';
+import { SupersetClient } from '@superset-ui/connection';
+
+import { sliceUpdated } from '../actions/exploreActions';
+import getClientErrorObject from '../../utils/getClientErrorObject';
+
+function PropertiesModalWrapper({ show, onHide, animation, slice, onSave }) {
+ // The wrapper is a separate component so that hooks only run when the modal opens
+ return (
+ <Modal show={show} onHide={onHide} animation={animation} bsSize="large">
+ <PropertiesModal slice={slice} onHide={onHide} onSave={onSave} />
+ </Modal>
+ );
+}
+
+function PropertiesModal({ slice, onHide, onSave }) {
+ const [submitting, setSubmitting] = useState(false);
+ const errorDialog = useRef();
+
+ function showError({ error, statusText }) {
+ errorDialog.current.show({
+ title: 'Error',
+ bsSize: 'medium',
+ bsStyle: 'danger',
+ actions: [Dialog.DefaultAction('Ok', () => {}, 'btn-danger')],
+ body: error || statusText || t('An error has occurred'),
+ });
+ }
+
+ // values of form inputs
+ const [name, setName] = useState(slice.slice_name || '');
+ const [description, setDescription] = useState(slice.description || '');
+ const [cacheTimeout, setCacheTimeout] = useState(slice.cache_timeout || '');
+ const [owners, setOwners] = useState(null);
+
+ async function fetchOwners() {
+ try {
+ const res = await SupersetClient.get({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ });
+ setOwners(
+ res.json.result.owners.map(owner => ({
+ value: owner.id,
+ label: owner.username,
+ })),
+ );
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ }
+
+ // get the owners of this slice
+ useEffect(() => {
+ fetchOwners();
+ }, []);
+
+ // get the list of users who can own a chart
+ const [ownerOptions, setUserOptions] = useState(null);
+ useEffect(() => {
+ SupersetClient.get({
+ endpoint: `/api/v1/chart/related/owners`,
+ }).then(res => {
+ setUserOptions(
+ res.json.result.map(item => ({
+ value: item.value,
+ label: item.text,
+ })),
+ );
+ });
+ }, []);
+
+ const onSubmit = async event => {
+ event.stopPropagation();
+ event.preventDefault();
+ setSubmitting(true);
+ const payload = {
+ slice_name: name || null,
+ description: description || null,
+ cache_timeout: cacheTimeout || null,
+ owners: owners.map(o => o.value),
+ };
+ try {
+ const res = await SupersetClient.put({
+ endpoint: `/api/v1/chart/${slice.slice_id}`,
+ headers: { 'Content-Type': 'application/json' },
+ body: JSON.stringify(payload),
+ });
+ // update the redux state
+ onSave(res.json.result);
+ onHide();
+ } catch (res) {
+ const errObj = await getClientErrorObject(res);
+ showError(errObj);
+ }
+ setSubmitting(false);
+ };
+
+ return (
+ <form onSubmit={onSubmit}>
+ <Modal.Header closeButton>
+ <Modal.Title>Edit Chart Properties</Modal.Title>
+ </Modal.Header>
+ <Modal.Body>
+ <Row>
+ <Col md={6}>
+ <h3>{t('Basic Information')}</h3>
+ <FormGroup>
+ <label className="control-label" htmlFor="name">
+ {t('Name')}
+ </label>
+ <FormControl
+ name="name"
+ type="text"
+ bsSize="sm"
+ value={name}
+ onChange={event => setName(event.target.value)}
+ />
+ </FormGroup>
+ <FormGroup>
+ <label className="control-label" htmlFor="description">
+ {t('Description')}
+ </label>
+ <FormControl
+ name="description"
+ type="text"
+ componentClass="textarea"
+ bsSize="sm"
+ value={description}
+ onChange={event => setDescription(event.target.value)}
+ style={{ maxWidth: '100%' }}
Review comment:
Inline styles!? Not sure the exact issue this addresses, but we can probably just slap this on all FormControl components via the theme if needed, so it needn't be repeated in the future.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org