Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Dataset Creation Footer Component #21241

Merged
merged 8 commits into from
Sep 23, 2022

Conversation

AAfghahi
Copy link
Member

@AAfghahi AAfghahi commented Aug 29, 2022

SUMMARY

This creates the footer component for the new dataset creation flow. Currently it does not set the name of the dataset, so default naming is used. However, this will be added after this: #21189 is merged in.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2022-09-08.at.3.39.05.PM.mov

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • 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

@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch from 3885487 to e635fa0 Compare August 29, 2022 16:43
@codecov
Copy link

codecov bot commented Aug 29, 2022

Codecov Report

Merging #21241 (8e2b2c1) into master (f27e20e) will decrease coverage by 0.02%.
The diff coverage is 52.83%.

@@            Coverage Diff             @@
##           master   #21241      +/-   ##
==========================================
- Coverage   66.66%   66.64%   -0.03%     
==========================================
  Files        1793     1793              
  Lines       68499    68852     +353     
  Branches     7278     7415     +137     
==========================================
+ Hits        45666    45886     +220     
- Misses      20969    21067      +98     
- Partials     1864     1899      +35     
Flag Coverage Δ
hive 53.08% <ø> (ø)
javascript 52.88% <52.83%> (+0.06%) ⬆️
mysql 78.20% <ø> (ø)
postgres 78.27% <ø> (ø)
presto 52.98% <ø> (ø)
python 81.41% <ø> (ø)
sqlite 76.76% <ø> (ø)
unit 51.06% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...iews/CRUD/data/dataset/AddDataset/Footer/index.tsx 33.33% <33.33%> (-66.67%) ⬇️
...s/CRUD/data/dataset/AddDataset/LeftPanel/index.tsx 47.61% <54.54%> (+0.34%) ⬆️
superset-frontend/src/logger/LogUtils.ts 96.00% <100.00%> (+1.26%) ⬆️
...d/src/views/CRUD/data/dataset/AddDataset/index.tsx 52.94% <100.00%> (+10.08%) ⬆️
...set-frontend/src/views/CRUD/data/dataset/styles.ts 100.00% <100.00%> (ø)
superset-frontend/src/SqlLab/constants.ts 100.00% <0.00%> (ø)
...src/dashboard/components/PropertiesModal/index.tsx 61.97% <0.00%> (+0.15%) ⬆️
...et-frontend/src/components/TableSelector/index.tsx 80.00% <0.00%> (+4.00%) ⬆️
...frontend/src/SqlLab/components/SqlEditor/index.jsx 56.90% <0.00%> (+5.48%) ⬆️
...tend/plugins/plugin-chart-table/src/TableChart.tsx 48.33% <0.00%> (+7.87%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 31, 2022
@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch from b7d4731 to dc07a5c Compare August 31, 2022 15:39
@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch from dc07a5c to ed55343 Compare September 7, 2022 21:00
@AAfghahi AAfghahi marked this pull request as ready for review September 8, 2022 19:41
@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch from e09362a to 18d86fc Compare September 8, 2022 19:49
@pkdotson
Copy link
Member

pkdotson commented Sep 8, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

@pkdotson Container image not yet published for this PR. Please try again when build is complete.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

@pkdotson Ephemeral environment creation failed. Please check the Actions logs for details.

@AAfghahi AAfghahi changed the title feat: rough draft of footer component feat: Dataset Creation Footer Component Sep 8, 2022
@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch from 18d86fc to c2aea81 Compare September 8, 2022 19:55

export default function Footer() {
return <div>Footer</div>;
interface FooterObject {

Choose a reason for hiding this comment

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

Could we rename this to be FooterProps? So it's more consistent with how we name our prop interfaces in other components.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Member

Choose a reason for hiding this comment

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

+1

@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch from c2aea81 to 7ff2200 Compare September 8, 2022 20:00
@@ -253,7 +273,14 @@ export default function LeftPanel({
<div className="options-list" data-test="options-list">
{!refresh &&
tableOptions.map((o, i) => (
Copy link
Member

@hughhhh hughhhh Sep 8, 2022

Choose a reason for hiding this comment

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

label this as option instead of o, and idx instead of i

Copy link
Member Author

Choose a reason for hiding this comment

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

done

describe('Footer', () => {
it('renders a blank state Footer', () => {
render(<Footer />);

Choose a reason for hiding this comment

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

It seems like we were rendering this component before without required props, and now, url and addDangerToast are required. Question, are there any other call to this component from before that was not updated to send the new props?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good question. This was a test that was created as a placeholder while we were implementing the blank state PR. Now that it is finished we are changing it to be reflective of the actual behavior.

}

function Footer({ url, datasetObject, addDangerToast }: FooterObject) {

Choose a reason for hiding this comment

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

I just realize we have two Footer components:
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/Footer/Footer.tsx
superset-frontend/src/views/CRUD/data/dataset/AddDataset/Footer/index.tsx

Isn't weird to have two components called<Footer> with different FooterProps in the same codebase? Should we consider a rename for one of them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is that weird. One of these is the footer for the native Filter the other is for AddDataset. I think that the folder structure helps differentiate them. How would you rename them?

Choose a reason for hiding this comment

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

Well, it depends, if we are keeping them separately like two individual Footers, I'd probably Call them FilterFooter and DatasetFooter or similar, so I know which Footer I'm working with. But yeah, the path is descriptive as you mention, not a blocker, more likely just personal taste, so nvm.

@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch 2 times, most recently from 6804515 to c9a8954 Compare September 8, 2022 21:57
Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

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

LGTM!

@AAfghahi
Copy link
Member Author

/testenv up

@github-actions
Copy link
Contributor

@AAfghahi Ephemeral environment spinning up at http://54.184.221.243:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@pkdotson pkdotson left a comment

Choose a reason for hiding this comment

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

lgtm!

@AAfghahi AAfghahi force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch 3 times, most recently from bb2af99 to c365cb0 Compare September 14, 2022 17:14
@@ -56,6 +65,14 @@ export const LOG_EVENT_TYPE_USER = new Set([
LOG_ACTIONS_MOUNT_EXPLORER,
]);

export const LOG_EVENT_DATASET_TYPE_DATASET_CREATION = new Set([
Copy link
Member

Choose a reason for hiding this comment

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

what is this set used for?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I was trying to figure that out as well, because you end up just importing them straight into the component either way. It was the pattern used by the other logging actions above so I copied it, but those sets are also not invoked anywhere. Happy to delete.

Copy link
Member

Choose a reason for hiding this comment

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

I removed the set in this commit.

@jinghua-qa
Copy link
Member

jinghua-qa commented Sep 15, 2022

Found 2 issues:
1, there is empty line between the search box and the table list, is this expected?
Screen Shot 2022-09-14 at 10 52 55 PM

2, when user searched a non-existed table name and deleted the name before table list refresh, the selected database table section will be refreshed too, is this expected?

delete.table.search.mov

@jinghua-qa
Copy link
Member

One more suggestion:
can we add a delete button to remove search word in the search box?
Screen Shot 2022-09-14 at 11 59 34 PM

@lyndsiWilliams lyndsiWilliams force-pushed the arash.afghahi/sc-52806/create-dataset-footer branch from c365cb0 to 8e2b2c1 Compare September 20, 2022 14:15
@lyndsiWilliams
Copy link
Member

2, when user searched a non-existed table name and deleted the name before table list refresh, the selected database table section will be refreshed too, is this expected?

delete.table.search.mov

@jinghua-qa The left panel initially used server side searching, which may have cause this. Server side searching was disabled recently so currently the table search isn't working. I'll be fixing the search so that it uses client side searching in my next PR, I will watch for this behavior while I'm fixing it but the search will not work in this PR.

For clarity: The entire table list should still display once a schema is chosen, you just can't search through it until my next fix PR.

@lyndsiWilliams
Copy link
Member

/testenv up

@github-actions
Copy link
Contributor

@lyndsiWilliams Ephemeral environment spinning up at http://35.92.221.221:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@lyndsiWilliams lyndsiWilliams added the need:qa-review Requires QA review label Sep 20, 2022
@lyndsiWilliams
Copy link
Member

@jinghua-qa I addressed all your comments above, could you take a look and make sure everything looks right now?

@jinghua-qa
Copy link
Member

jinghua-qa commented Sep 23, 2022

Re-test in http://35.92.221.221:8080/, find an issue that the table list did not change with the search keyword:
repo steps:
1, input keyword in the search box
expect:
table list will show match result with keyword
actual:
table list did not change

Screen.Recording.2022-09-23.at.12.32.23.AM.mov

@lyndsiWilliams
Copy link
Member

Re-test in http://35.92.221.221:8080/, find an issue that the table list did not change with the search keyword: repo steps: 1, input keyword in the search box expect: table list will show match result with keyword actual: table list did not change

Screen.Recording.2022-09-23.at.12.32.23.AM.mov

Yes, I mentioned this a couple of comments above. Server side searching was disabled recently so currently the table search isn't working. I'll be fixing the search so that it uses client side searching in my next PR.

@jinghua-qa
Copy link
Member

Re-test in http://35.92.221.221:8080/, find an issue that the table list did not change with the search keyword: repo steps: 1, input keyword in the search box expect: table list will show match result with keyword actual: table list did not change
Screen.Recording.2022-09-23.at.12.32.23.AM.mov

Yes, I mentioned this a couple of comments above. Server side searching was disabled recently so currently the table search isn't working. I'll be fixing the search so that it uses client side searching in my next PR.

Sorry have missed your previous comment, i think other than that i did not find other issues~ LGTM

@lyndsiWilliams lyndsiWilliams merged commit c4638fa into master Sep 23, 2022
@lyndsiWilliams lyndsiWilliams deleted the arash.afghahi/sc-52806/create-dataset-footer branch September 23, 2022 23:40
@github-actions
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels need:qa-review Requires QA review size/L 🚢 2.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants