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 2022/09/22 20:30:37 UTC
[GitHub] [superset] lyndsiWilliams opened a new pull request, #21557: fix: Dataset creation header is now uneditable and holds proper default values
lyndsiWilliams opened a new pull request, #21557:
URL: https://github.com/apache/superset/pull/21557
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Dataset header name is now uneditable. It displays "New dataset' until a table is selected, then it displays the selected table's name.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
#### BEFORE:
#### AFTER:
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
- Go to `http://localhost:9000/dataset/add/?testing`
- Try to click the header and see that it is no longer editable (at any point)
- Select a database, schema, and table name
- Observe that the header now displays the table name
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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
[GitHub] [superset] lyndsiWilliams commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1261625298
/testenv up
--
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
[GitHub] [superset] eschutho commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r979089492
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,7 +71,11 @@ export default function AddDataset() {
>(datasetReducer, null);
const HeaderComponent = () => (
- <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+ <Header
+ setDataset={setDataset}
+ title={dataset?.table_name ?? 'New dataset'}
Review Comment:
do you want this title to be 'New dataset' if dataset.table_name is undefined or ''?
--
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
[GitHub] [superset] eschutho commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
eschutho commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r982800312
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,7 +71,11 @@ export default function AddDataset() {
>(datasetReducer, null);
const HeaderComponent = () => (
- <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+ <Header
+ setDataset={setDataset}
+ title={dataset?.table_name ?? 'New dataset'}
Review Comment:
Ok, what you have is good!
--
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
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r979119689
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,7 +71,11 @@ export default function AddDataset() {
>(datasetReducer, null);
const HeaderComponent = () => (
- <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+ <Header
+ setDataset={setDataset}
+ title={dataset?.table_name ?? 'New dataset'}
Review Comment:
Actually, depending on the intended header behavior, we might need to figure something else out. If I set the default here to "Undefined dataset" it will temporarily change to that once a schema is chosen and a table name isn't selected. This could be okay as well though, because _technically_ it is undefined at that point. What do you think?
--
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
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r979087475
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,13 +71,17 @@ export default function AddDataset() {
>(datasetReducer, null);
const HeaderComponent = () => (
- <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+ <Header
+ setDataset={setDataset}
+ title={dataset?.table_name ?? 'New dataset'}
+ schema={dataset?.schema ?? ''}
+ />
);
const LeftPanelComponent = () => (
<LeftPanel
setDataset={setDataset}
- schema={dataset?.schema}
+ schema={dataset?.schema ?? ''}
Review Comment:
Good call, that does make more sense! Fixed in [`this commit`](https://github.com/apache/superset/pull/21557/commits/7fa31ba609eadb2b9d01a884abc8da57a053ee79).
--
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
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r979106792
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,7 +71,11 @@ export default function AddDataset() {
>(datasetReducer, null);
const HeaderComponent = () => (
- <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+ <Header
+ setDataset={setDataset}
+ title={dataset?.table_name ?? 'New dataset'}
Review Comment:
Oh good point, I didn't think of undefined table names. How would that table show up in the left panel, in that case? Would it be a blank line? Maybe a good default would be "Undefined dataset"?
--
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
[GitHub] [superset] michael-s-molina commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1271910673
It's looking so beautiful :heart_eyes:
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1278247143
Ephemeral environment shutdown and build artifacts deleted.
--
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
[GitHub] [superset] lyndsiWilliams merged pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams merged PR #21557:
URL: https://github.com/apache/superset/pull/21557
--
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
[GitHub] [superset] lyndsiWilliams commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1255567510
/testenv up
--
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
[GitHub] [superset] codecov[bot] commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1255523255
# [Codecov](https://codecov.io/gh/apache/superset/pull/21557?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21557](https://codecov.io/gh/apache/superset/pull/21557?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (895abb4) into [arash.afghahi/sc-52806/create-dataset-footer](https://codecov.io/gh/apache/superset/commit/8e2b2c113f4e7080a76e1dff106ffccf43686104?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8e2b2c1) will **decrease** coverage by `0.74%`.
> The diff coverage is `80.00%`.
> :exclamation: Current head 895abb4 differs from pull request most recent head c4923c3. Consider uploading reports for the commit c4923c3 to get more accurate results
```diff
@@ Coverage Diff @@
## arash.afghahi/sc-52806/create-dataset-footer #21557 +/- ##
================================================================================
- Coverage 66.64% 65.89% -0.75%
================================================================================
Files 1793 1793
Lines 68852 68852
Branches 7415 7415
================================================================================
- Hits 45886 45373 -513
- Misses 21067 21580 +513
Partials 1899 1899
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `53.08% <ø> (ø)` | |
| mysql | `?` | |
| postgres | `?` | |
| presto | `52.98% <ø> (ø)` | |
| python | `79.86% <ø> (-1.55%)` | :arrow_down: |
| sqlite | `76.76% <ø> (ø)` | |
| unit | `51.06% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/21557?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...s/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvTGVmdFBhbmVsL2luZGV4LnRzeA==) | `47.61% <ø> (ø)` | |
| [...iews/CRUD/data/dataset/AddDataset/Header/index.tsx](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvSGVhZGVyL2luZGV4LnRzeA==) | `87.50% <50.00%> (ø)` | |
| [...d/src/views/CRUD/data/dataset/AddDataset/index.tsx](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhc2V0L0FkZERhdGFzZXQvaW5kZXgudHN4) | `52.94% <100.00%> (ø)` | |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `31.25% <0.00%> (-68.75%)` | :arrow_down: |
| [superset/datasets/commands/bulk\_delete.py](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvYnVsa19kZWxldGUucHk=) | `33.33% <0.00%> (-53.34%)` | :arrow_down: |
| [superset/datasets/columns/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29sdW1ucy9jb21tYW5kcy9kZWxldGUucHk=) | `44.11% <0.00%> (-52.95%)` | :arrow_down: |
| [superset/datasets/metrics/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvbWV0cmljcy9jb21tYW5kcy9kZWxldGUucHk=) | `44.11% <0.00%> (-52.95%)` | :arrow_down: |
| [superset/datasets/dao.py](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvZGFvLnB5) | `44.21% <0.00%> (-50.35%)` | :arrow_down: |
| [superset/sql\_validators/postgres.py](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3FsX3ZhbGlkYXRvcnMvcG9zdGdyZXMucHk=) | `50.00% <0.00%> (-50.00%)` | :arrow_down: |
| ... and [31 more](https://codecov.io/gh/apache/superset/pull/21557/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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
[GitHub] [superset] eric-briscoe commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
eric-briscoe commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r983904475
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,7 +71,11 @@ export default function AddDataset() {
>(datasetReducer, null);
const HeaderComponent = () => (
- <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+ <Header
+ setDataset={setDataset}
+ title={dataset?.table_name ?? 'New dataset'}
Review Comment:
May want to wrap in a t('New dataset')
Also, may be worth using exported const from previous comment if you want to keep this in sync with what the default title is for header.tsx.
If making the title prop optional like mentioned in comment on Header/index.tsx then this could be simplified to `title={dataset?.table_name}` and the default value 'New dataset' would get applied by Header and schema prop could be removed
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Header/index.tsx:
##########
@@ -59,21 +59,23 @@ const renderOverlay = () => (
export default function Header({
setDataset,
- datasetName,
+ title,
+ schema,
}: {
setDataset: React.Dispatch<DSReducerActionType>;
- datasetName: string;
+ title: string;
+ schema?: string | null | undefined;
}) {
const editableTitleProps = {
- title: datasetName,
- placeholder: t('Add the name of the dataset'),
+ title: schema ? title : t('New dataset'),
+ placeholder: t('New dataset'),
Review Comment:
Since you are using t'(New dataset') twice here, it may be worth defining a const with that value and using it in both places. If you export that const you could also use it in the test file so that if someone changes the value later you don't have to keep three separate string literals in sync manually.
export const DEFAULT_TITLE = t(New dataset);
Then use that on line 70, 71, and in line 51 of Header.test.tsx
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Header/index.tsx:
##########
@@ -59,21 +59,23 @@ const renderOverlay = () => (
export default function Header({
setDataset,
- datasetName,
+ title,
+ schema,
}: {
setDataset: React.Dispatch<DSReducerActionType>;
- datasetName: string;
+ title: string;
+ schema?: string | null | undefined;
}) {
const editableTitleProps = {
- title: datasetName,
- placeholder: t('Add the name of the dataset'),
+ title: schema ? title : t('New dataset'),
Review Comment:
curious why the usage of title here is tied to if schema is defined? If you make title optional, then set a default value for title as t('New dataset') could the conditional logic here could be removed?
`
export const DEFAULT_TITLE = t(New dataset);
export default function Header({
setDataset,
title = DEFAULT_TITLE,
}: {
setDataset: React.Dispatch<DSReducerActionType>;
title?: string;
}) {
const editableTitleProps = {
title,
placeholder: DEFAULT_TITLE
`
It looks like the only use of schema prop is this case so changing this logic might also make the prop unnecessary and simplify the component's interface
##########
superset-frontend/src/views/CRUD/data/dataset/DatasetLayout/DatasetLayout.test.tsx:
##########
@@ -36,18 +36,12 @@ describe('DatasetLayout', () => {
const mockSetDataset = jest.fn();
const waitForRender = () =>
- waitFor(() =>
- render(<Header setDataset={mockSetDataset} datasetName="" />),
- );
+ waitFor(() => render(<Header setDataset={mockSetDataset} title="" />));
Review Comment:
I think the other comments I left might address this and allow the title="" to be removed
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1275099578
@jinghua-qa Ephemeral environment spinning up at http://54.202.251.181:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r979041872
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,13 +71,17 @@ export default function AddDataset() {
>(datasetReducer, null);
const HeaderComponent = () => (
- <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+ <Header
+ setDataset={setDataset}
+ title={dataset?.table_name ?? 'New dataset'}
+ schema={dataset?.schema ?? ''}
+ />
);
const LeftPanelComponent = () => (
<LeftPanel
setDataset={setDataset}
- schema={dataset?.schema}
+ schema={dataset?.schema ?? ''}
Review Comment:
Since `schema` is optional as per the definition in `LeftPanelProps` wouldn't be sufficient to just send `dataset?.schema` ? without the empty string.
##########
superset-frontend/src/views/CRUD/data/dataset/DatasetLayout/DatasetLayout.test.tsx:
##########
@@ -36,18 +36,12 @@ describe('DatasetLayout', () => {
const mockSetDataset = jest.fn();
const waitForRender = () =>
- waitFor(() =>
- render(<Header setDataset={mockSetDataset} datasetName="" />),
- );
+ waitFor(() => render(<Header setDataset={mockSetDataset} title="" />));
Review Comment:
Isn't this the same as having no title thus the `title: string;` should be optional? Or what implication would that have? 🤔
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1255581330
@lyndsiWilliams Ephemeral environment spinning up at http://18.236.78.16:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
[GitHub] [superset] jinghua-qa commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1274996314
/testenv up
--
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
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r979088255
##########
superset-frontend/src/views/CRUD/data/dataset/DatasetLayout/DatasetLayout.test.tsx:
##########
@@ -36,18 +36,12 @@ describe('DatasetLayout', () => {
const mockSetDataset = jest.fn();
const waitForRender = () =>
- waitFor(() =>
- render(<Header setDataset={mockSetDataset} datasetName="" />),
- );
+ waitFor(() => render(<Header setDataset={mockSetDataset} title="" />));
Review Comment:
The title can't have nullish values because of the component that uses it: [PageHeaderWithActions](https://github.com/apache/superset/blob/master/superset-frontend/src/components/PageHeaderWithActions/index.tsx)
<img width="707" alt="Screen Shot 2022-09-23 at 4 45 18 PM" src="https://user-images.githubusercontent.com/55605634/192060971-27fa04cc-4ff5-4b84-85ff-4168df8327ab.png">
--
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
[GitHub] [superset] github-actions[bot] commented on pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21557:
URL: https://github.com/apache/superset/pull/21557#issuecomment-1261627481
@lyndsiWilliams Ephemeral environment spinning up at http://35.91.137.104:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #21557: fix: Dataset creation header is now uneditable and holds proper default values
Posted by GitBox <gi...@apache.org>.
lyndsiWilliams commented on code in PR #21557:
URL: https://github.com/apache/superset/pull/21557#discussion_r989250489
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Header/index.tsx:
##########
@@ -59,21 +59,23 @@ const renderOverlay = () => (
export default function Header({
setDataset,
- datasetName,
+ title,
+ schema,
}: {
setDataset: React.Dispatch<DSReducerActionType>;
- datasetName: string;
+ title: string;
+ schema?: string | null | undefined;
}) {
const editableTitleProps = {
- title: datasetName,
- placeholder: t('Add the name of the dataset'),
+ title: schema ? title : t('New dataset'),
+ placeholder: t('New dataset'),
Review Comment:
Updated in [`this commit`](https://github.com/apache/superset/pull/21557/commits/e9f6eb28b1a684b08eb097d43dee99034d775df8)
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Header/index.tsx:
##########
@@ -59,21 +59,23 @@ const renderOverlay = () => (
export default function Header({
setDataset,
- datasetName,
+ title,
+ schema,
}: {
setDataset: React.Dispatch<DSReducerActionType>;
- datasetName: string;
+ title: string;
+ schema?: string | null | undefined;
}) {
const editableTitleProps = {
- title: datasetName,
- placeholder: t('Add the name of the dataset'),
+ title: schema ? title : t('New dataset'),
Review Comment:
Updated in [`this commit`](https://github.com/apache/superset/pull/21557/commits/e9f6eb28b1a684b08eb097d43dee99034d775df8)
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/index.tsx:
##########
@@ -71,7 +71,11 @@ export default function AddDataset() {
>(datasetReducer, null);
const HeaderComponent = () => (
- <Header setDataset={setDataset} datasetName={dataset?.dataset_name ?? ''} />
+ <Header
+ setDataset={setDataset}
+ title={dataset?.table_name ?? 'New dataset'}
Review Comment:
Updated in [`this commit`](https://github.com/apache/superset/pull/21557/commits/e9f6eb28b1a684b08eb097d43dee99034d775df8)
--
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