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 2021/07/12 19:53:52 UTC
[GitHub] [superset] betodealmeida commented on a change in pull request #15598: feat: Gsheet adding sheets component
betodealmeida commented on a change in pull request #15598:
URL: https://github.com/apache/superset/pull/15598#discussion_r668149662
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -115,78 +121,90 @@ const CredentialsInfo = ({
placeholder="Paste content of service credentials JSON file here"
/>
<span className="label-paste">
- {t('Copy and paste the entire service account .json file here')}
+ {t(
+ 'Copy and paste the entire service account .json file here to enable connection',
Review comment:
We should reuse the same component we have for uploading/pasting credentials in BQ.
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -115,78 +121,90 @@ const CredentialsInfo = ({
placeholder="Paste content of service credentials JSON file here"
/>
<span className="label-paste">
- {t('Copy and paste the entire service account .json file here')}
+ {t(
+ 'Copy and paste the entire service account .json file here to enable connection',
Review comment:
We should use the same copy as BQ here.
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/index.tsx
##########
@@ -1069,7 +1082,9 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
</StyledAlertMargin>
)}
<DatabaseConnectionForm
+ isPublic={isPublic}
db={db}
+ setPublicSheets={setPublicSheets}
Review comment:
Nit, since these two are related let's keep them together:
```suggestion
db={db}
isPublic={isPublic}
setPublicSheets={setPublicSheets}
```
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -299,19 +317,42 @@ const displayField = ({
getValidation,
validationErrors,
db,
+ setPublicSheets,
}: FieldPropTypes) => (
- <ValidatedInput
- id="database_name"
- name="database_name"
- required
- value={db?.database_name}
- validationMethods={{ onBlur: getValidation }}
- errorMessage={validationErrors?.database_name}
- placeholder=""
- label="Display Name"
- onChange={changeMethods.onChange}
- helpText={t('Pick a nickname for this database to display as in Superset.')}
- />
+ <>
+ <ValidatedInput
+ id="database_name"
+ name="database_name"
+ required
+ value={db?.database_name}
+ validationMethods={{ onBlur: getValidation }}
+ errorMessage={validationErrors?.database_name}
+ placeholder=""
+ label="Display Name"
+ onChange={changeMethods.onChange}
+ helpText={t(
+ 'Pick a nickname for this database to display as in Superset.',
+ )}
+ />
+
+ {db?.engine === 'gsheets' && (
+ <>
+ <FormLabel required>{t('Type of Google Sheets Allowed')}</FormLabel>
+ <Select
+ style={{ width: '100%' }}
+ onChange={(value: string) => setPublicSheets(value)}
+ defaultValue="true"
+ >
+ <Select.Option value="true" key={1}>
+ Publicly shared sheets only
+ </Select.Option>
+ <Select.Option value="false" key={2}>
+ Public and privately shared sheets
+ </Select.Option>
Review comment:
If you set the values to booleans you don't need to convert them from string:
```suggestion
<Select.Option value={true} key={1}>
Publicly shared sheets only
</Select.Option>
<Select.Option value={false} key={2}>
Public and privately shared sheets
</Select.Option>
```
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -115,78 +121,90 @@ const CredentialsInfo = ({
placeholder="Paste content of service credentials JSON file here"
/>
<span className="label-paste">
- {t('Copy and paste the entire service account .json file here')}
+ {t(
+ 'Copy and paste the entire service account .json file here to enable connection',
Review comment:
Also, I'm a bit worried that we're adding conditional rendering based on the engine name, not on its parameters — this will quickly become unmaintainable as we add more databases.
Maybe we could create separate files for custom DBs like BQ, GSheets?
##########
File path: superset-frontend/src/views/CRUD/data/database/DatabaseModal/DatabaseConnectionForm.tsx
##########
@@ -115,78 +121,90 @@ const CredentialsInfo = ({
placeholder="Paste content of service credentials JSON file here"
/>
<span className="label-paste">
- {t('Copy and paste the entire service account .json file here')}
+ {t(
+ 'Copy and paste the entire service account .json file here to enable connection',
+ )}
</span>
</div>
) : (
- <div
- className="input-container"
- css={(theme: SupersetTheme) => infoTooltip(theme)}
- >
- <div css={{ display: 'flex', alignItems: 'center' }}>
- <FormLabel required>{t('Upload Credentials')}</FormLabel>
- <InfoTooltip
- tooltip={t(
- 'Use the JSON file you automatically downloaded when creating your service account in Google BigQuery.',
- )}
- viewBox="0 0 24 24"
- />
- </div>
-
- {!fileToUpload && (
- <Button
- className="input-upload-btn"
- onClick={() => document?.getElementById('selectedFile')?.click()}
- >
- {t('Choose File')}
- </Button>
- )}
- {fileToUpload && (
- <div className="input-upload-current">
- {fileToUpload}
- <DeleteFilled
- onClick={() => {
- setFileToUpload(null);
- changeMethods.onParametersChange({
- target: {
- name: 'credentials_info',
- value: '',
- },
- });
- }}
+ db?.engine !== 'gsheets' && (
Review comment:
```suggestion
db?.engine === 'bigquery' && (
```
We don't want to risk showing the copy below ('Use the JSON file you automatically downloaded when creating your service account in Google BigQuery.') for non-BQ databases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org