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(dashboard): Add metadata bar to the header #27857

Merged

Conversation

justinpark
Copy link
Member

@justinpark justinpark commented Apr 2, 2024

SUMMARY

As the dashboard currently lacks metadata information such as owners and last modified time, users have to navigate to edit mode and click "edit properties" to access this information. Unfortunately, there is no way to check the last modified time at all.
To address this issue, this commit integrates the metadata bar UI from the chart header into the dashboard header and updates the API properties accordingly. This enhancement will provide users with convenient access to the metadata information they need.

Screenshot 2024-04-02 at 1 24 09 PM

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

after--dashboard-metadata.mov

TESTING INSTRUCTIONS

Go to a dashboard and then check the header

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

@mistercrunch
Copy link
Member

Do we / should we hide this in print mode (standalone=true) or in an embedded context? I'm not sure what the behavior should be or what the options settings should be and generally think the more information the better, but I'm unclear if everyone would agree.

@rusackas
Copy link
Member

rusackas commented Apr 3, 2024

+1 on the more the merrier. Knowing how fresh the dashboard is (the edit timestamp) is valuable even in an embedded context. The owners might not be something you want to see in an embedded context, but if anyone barks about that, I think that's also addressable — probably by means of a new config option (prop?) in the embedded component, just like you'd tweak other features (filter bar collapsing, etc)

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

Code LGTM. Further discussion about the feature's utility in embedding may be warranted (CC @kasiazjc @yousoph) but otherwise, I'm onboard :D

@michael-s-molina
Copy link
Member

/testenv up

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 improvement @justinpark.

Comment on lines 253 to 261
@property
def created_by_name(self) -> str:
if not self.created_by:
return ""
return str(self.created_by)

@property
def owners_by_name(self) -> list[str]:
return [owner.get_full_name() for owner in self.owners]
Copy link
Member

Choose a reason for hiding this comment

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

This looks like more of frontend code than a new model property. If you check other instances of the Metadata component, this is being handled on the frontend as we may have different representations of the possible values.

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

Copy link
Contributor

github-actions bot commented Apr 3, 2024

@michael-s-molina Ephemeral environment spinning up at http://34.208.49.238:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

@justinpark justinpark force-pushed the feat--add-metadata-bar-on-dashboard branch from 9b0cb6c to 8dd320c Compare May 8, 2024 17:54
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 60.80%. Comparing base (76d897e) to head (65d2640).
Report is 70 commits behind head on master.

Files Patch % Lines
...frontend/src/dashboard/components/Header/index.jsx 57.14% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #27857      +/-   ##
==========================================
+ Coverage   60.48%   60.80%   +0.31%     
==========================================
  Files        1931     1937       +6     
  Lines       76236    76924     +688     
  Branches     8568     8606      +38     
==========================================
+ Hits        46114    46774     +660     
- Misses      28017    28048      +31     
+ Partials     2105     2102       -3     
Flag Coverage Δ
hive 49.08% <100.00%> (-0.09%) ⬇️
presto 53.67% <100.00%> (-0.14%) ⬇️
python 64.14% <100.00%> (+0.65%) ⬆️
unit 58.35% <100.00%> (+0.73%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@justinpark
Copy link
Member Author

Do we / should we hide this in print mode (standalone=true) or in an embedded context? I'm not sure what the behavior should be or what the options settings should be and generally think the more information the better, but I'm unclear if everyone would agree.

Since we currently do not hide any items in the header in standalone mode, let's keep the metadata bar as it is for the time being.

@@ -242,6 +242,12 @@ def changed_by_name(self) -> str:
return ""
return str(self.changed_by)

@property
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the same case as the owners and should be handled by the frontend.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good

@github-actions github-actions bot removed the api Related to the REST API label May 8, 2024
@justinpark justinpark merged commit 02478e5 into apache:master May 10, 2024
34 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@nemccarthy
Copy link

nemccarthy commented Sep 9, 2024

Do we / should we hide this in print mode (standalone=true) or in an embedded context? I'm not sure what the behavior should be or what the options settings should be and generally think the more information the better, but I'm unclear if everyone would agree.

This is a great call out and we 100% need this. In the embedded context why would someone need to know about metadata thats really useful for a superset user? You are exposing details about superset and its users to an external application. It just causes confusion.

In addition Tags like "Published" and the fav icon can be hidden in embedding.

It should be an option and totally breaks embedding for us as we do not want to show this to end users in the embedded use case. Have raised issue #30188 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants