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: add database and schema names to dataset option #25569

Merged
merged 7 commits into from Oct 19, 2023

Conversation

soniagtm
Copy link
Contributor

@soniagtm soniagtm commented Oct 9, 2023

SUMMARY

This PR is created for adding database and schema names to the dataset option in the chart creation page and dashboard filter modal as described in Discussion: #25545.

The changes I have made will add database and schema names beneath each dataset name in the drop down list, as well as in the tooltip, so that users can identify the database and schema from which each dataset is created.

For datasets with a null schema name, the schema name in the tooltip label will be displayed as ‘Not defined’ instead.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE

Chart creation page

before-chart

Dashboard filter modal

before-dashboard

AFTER

Chart creation page

after-chart

Dashboard filter modal

after-dashboard

TESTING INSTRUCTIONS

To verify the changes made, please follow these steps:

Chart Creation Page:
(Assume that there exist some datasets that could be used to create a chart)

  1. Go to the Charts tab and click the “+ CHART” button on the top right to navigate to the chart creation page.
  2. Try choosing a dataset where you will find the database and schema names displayed below each dataset name.
  3. Hover over each option to see the tooltip displaying the dataset name and its corresponding database and schema names.

Dashboard Filter Modal:
(Assume that there are some dashboards to which you can apply filters)

  1. Go to the Dashboards tab and click on any dashboard to view it.
  2. Expand the left panel and click "Add/Edit Filter," which will open the Add and edit filters modal.
  3. Try selecting a dataset from the dropdown list, where you will find the database and schema names displayed below each dataset name.
  4. Hover over each option to see the tooltip displaying the dataset name and its corresponding database and schema names.

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

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Congrats on making your first PR and thank you for contributing to Superset! 🎉 ❤️

We hope to see you in our Slack community too! Not signed up? Use our Slack App to self-register.

@soniagtm soniagtm changed the title [WIP] feat: add database and schema names to dataset option feat: add database and schema names to dataset option Oct 9, 2023
@rusackas
Copy link
Member

rusackas commented Oct 10, 2023

Thanks for the PR! I'm curious what others (especially @kasiazjc) think about this. In particular, I'm wondering about users that have a whole lot of datasets. Adding the table/schema in the tooltip is AMAZING and (I think) uncontroversial. What I'm not sure about is if the table and schema are needed in the menu option listings, since they take up more vertical real estate, and will require more scrolling if the list is long. The tooltip seems sufficient to me, but I'm not sure ¯\_(ツ)_/¯

@kasiazjc
Copy link
Contributor

amazing @soniagtm and congrats on your first PR!

@soniagtm I have some feedback about the metadata in the dropdown (schema and database) - I think it's 14px, can you change it to 12px? This way it all will be less cluttered and we'll get back a little bit of the precious real estate @rusackas :D

For the context, @rusackas: Details in the dropdown solve a problem of quickly identifying which dataset is the one you're looking for (there are use cases of datasets with the same name, or users are just not familiar with them) or where does it come from, what is the context. That's why the request/solution was to make those details visible at all times. Also, this field is searchable, so it shouldn't be a problem with super long list.

@john-bodley
Copy link
Member

Another design option would be to replicate the database/schema UI elements we use in SQL Lab, etc. for consistency reasons, however it might be overkill, and/or a cumbersome UX.

@kasiazjc
Copy link
Contributor

Another design option would be to replicate the database/schema UI elements we use in SQL Lab, etc. for consistency reasons, however it might be overkill, and/or a cumbersome UX.

Yeah, I think it would be too much - we are actually thinking about using this pattern with metadata in the dropdown in other places, so for sure it wouldn't be a one time thing.

@soniagtm
Copy link
Contributor Author

Thanks to all of you for your feedback. I've reduced the font size as suggested, and also attached some screenshots to show how it looks now. Please have a look and let me know if there are any further improvements or adjustments needed :)

Chart creation page
Screenshot 2566-10-12 at 21 21 17


Dashboard filter modal
Screenshot 2566-10-12 at 21 23 27

@kasiazjc
Copy link
Contributor

Thanks to all of you for your feedback. I've reduced the font size as suggested, and also attached some screenshots to show how it looks now. Please have a look and let me know if there are any further improvements or adjustments needed :)

Chart creation page Screenshot 2566-10-12 at 21 21 17 Dashboard filter modal Screenshot 2566-10-12 at 21 23 27

This looks good to me! Thank you for all the changes :)

Copy link
Contributor

@kasiazjc kasiazjc left a comment

Choose a reason for hiding this comment

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

Design LGTM

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @soniagtm! I really think this brings a lot of value.

Currently, there are 2 concepts in Superset:
src/components contains components that are really generic and could be extracted to a library to be used outside of Superset.
src/features contains components that belong to the Superset domain and that are shared between multiple modules.

Given that we want to uniformly represent datasets throughout the application, the implementation of this new label should be moved to src/features/datasets and be reused in the native filters and chart creation.

@justinpark
Copy link
Member

One more UI issue. The labels are displayed partially after you select them.

Screenshot 2023-10-13 at 11 23 45 AM

@michael-s-molina
Copy link
Member

@soniagtm I verified that long database and schema names are truncated. Nice.

Screenshot 2023-10-13 at 13 21 53

We can improve the layout a little bit to use all the available space in some cases.

Screenshot 2023-10-13 at 13 20 49

@kasiazjc
Copy link
Contributor

One more UI issue. The labels are displayed partially after you select them.

Screenshot 2023-10-13 at 11 23 45 AM

Oh, good catch, thank you! From what we have defined in the design system - the metadata should not be displayed in the field

@michael-s-molina
Copy link
Member

michael-s-molina commented Oct 13, 2023

Oh, good catch, thank you! From what we have defined in the design system - the metadata should not be displayed in the field

We'll need to augment the Select component to support this as currently there's no distinction between the label that is displayed in the dropdown and the label that is displayed in the input. Let's leave this fix for last as it will give us time to work on the Select component part.

@soniagtm
Copy link
Contributor Author

I've moved the dataset label implementation to src/features/datasets as suggested and also fixed the UI issues found.


BEFORE

One more UI issue. The labels are displayed partially after you select them.

Screenshot 2023-10-13 at 11 23 45 AM

AFTER

Screenshot 2566-10-16 at 10 00 45

BEFORE

@soniagtm I verified that long database and schema names are truncated. Nice.

Screenshot 2023-10-13 at 13 21 53 We can improve the layout a little bit to use all the available space in some cases. Screenshot 2023-10-13 at 13 20 49

AFTER

Screenshot 2566-10-16 at 10 05 38


Please let me know if there are any issues or if any adjustments are needed.

Thanks!

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the comments @soniagtm. It's looking great! I left some more suggestions.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the awesome improvement @soniagtm and for patiently addressing all comments. Great job!

Copy link
Member

@justinpark justinpark left a comment

Choose a reason for hiding this comment

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

LGTM!

@soniagtm
Copy link
Contributor Author

Thank you for your kind words and for helping me with my first PR! I'm really grateful for all the suggestions :)

@rusackas rusackas merged commit 39ad322 into apache:master Oct 19, 2023
26 checks passed
@rusackas
Copy link
Member

Echoing everyone here, thank you for this submission and all the effort that went into it!

cccs-rc pushed a commit to CybercentreCanada/superset that referenced this pull request Mar 6, 2024
Co-authored-by: Sonia <sonia.gautam@agoda.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
sfirke pushed a commit to sfirke/superset that referenced this pull request Mar 22, 2024
Co-authored-by: Sonia <sonia.gautam@agoda.com>
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 size/L 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants