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

Fix can't select dashboard tabs in interactive embedding with parameter header=false #39007

Conversation

WiNloSt
Copy link
Member

@WiNloSt WiNloSt commented Feb 21, 2024

Note

This PR supersedes #39005

Closes #38429, #39002

Description

The header documentation indicates

To hide a question or dashboard’s title, additional info, and action buttons

Dashboard tabs is a very crucial part of the dashboard and should, in my opinion, never been hidden.

This PR fixes the problem by only hiding the other parts in the dashboard header except the tabs

Note Please also see the #39005 explaining the fix for the dashboard card not loading which is also fixed in this PR with a different approach.

How to verify

  1. Set up interactive embedding
  2. Create a dashboard with multiple tabs
  3. Visit the interactive embedding page where the iframe URL has this parameter ?header=false
  4. Visit the dashboard we created in 1)
  5. See that the tabs are visible and you can select each tab.

Demo

Before

image

After

image

Checklist

  • Tests have been added/updated to cover changes in this PR

/>
</DashboardHeaderContainer>
)}
<DashboardHeaderContainer
Copy link
Member Author

Choose a reason for hiding this comment

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

Pro tip: Use the hide whitespace option for EZ review.
image

data-testid="fixed-width-dashboard-header"
isNavBarOpen={isNavBarOpen}
isFixedWidth={dashboard?.width === "fixed"}
{isDashboardHeaderVisible && (
Copy link
Member Author

Choose a reason for hiding this comment

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

Only hide the part before the dashboard tabs, because the dashboard tabs should always be visible.

@WiNloSt WiNloSt added the backport Automatically create PR on current release branch on merge label Feb 21, 2024
@WiNloSt WiNloSt requested review from a team and albertoperdomo February 21, 2024 14:34
Copy link

replay-io bot commented Feb 21, 2024

Status Complete ↗︎
Commit 5014ea5
Results
⚠️ 3 Flaky
2315 Passed

Copy link
Member

@albertoperdomo albertoperdomo left a comment

Choose a reason for hiding this comment

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

LGTM

Related: I am getting the connect your database to get get the most from MB banner at the top in the embed, but I am also seeing that on master, so related but separate issue.

@WiNloSt WiNloSt merged commit 94795ab into master Feb 22, 2024
120 of 138 checks passed
@WiNloSt WiNloSt deleted the 38429-fix-interactive-embedding-with-header-false-can-not-change-tabs branch February 22, 2024 07:21
Copy link

@WiNloSt Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

WiNloSt added a commit that referenced this pull request Feb 22, 2024
…er `header=false` (#39007)

* Remove full-app to a newer term

* Fix dashboard not loading in interactive embedding with header=false
header=false

* Fix can't choose dashboard tabs in interactive embedding w/ header=false
@WiNloSt
Copy link
Member Author

WiNloSt commented Feb 22, 2024

@albertoperdomo Feel free to open an issue for that too. I remember we haven't talked about hiding that banner specifically in embedding, we only talked about hiding that database prompt banner when users are whitelabeling. That feels to me we should do that for interactive embedding as well, since interactive embedding is also closely related to whitelabeling.

WiNloSt added a commit that referenced this pull request Feb 22, 2024
…er `header=false` (#39007) (#39039)

* Remove full-app to a newer term

* Fix dashboard not loading in interactive embedding with header=false
header=false

* Fix can't choose dashboard tabs in interactive embedding w/ header=false

Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>
@WiNloSt
Copy link
Member Author

WiNloSt commented Mar 15, 2024

@metabase-bot backport release-x.48.x

Copy link

@WiNloSt something went wrong while creating a backport [Logs]

WiNloSt added a commit that referenced this pull request Mar 15, 2024
…er `header=false` (#39007)

* Remove full-app to a newer term

* Fix dashboard not loading in interactive embedding with header=false
header=false

* Fix can't choose dashboard tabs in interactive embedding w/ header=false
WiNloSt added a commit that referenced this pull request Mar 15, 2024
…er `header=false` (#39007)

* Remove full-app to a newer term

* Fix dashboard not loading in interactive embedding with header=false
header=false

* Fix can't choose dashboard tabs in interactive embedding w/ header=false
WiNloSt added a commit that referenced this pull request Mar 15, 2024
* Fix can't select dashboard tabs in interactive embedding with parameter `header=false` (#39007)

* Remove full-app to a newer term

* Fix dashboard not loading in interactive embedding with header=false
header=false

* Fix can't choose dashboard tabs in interactive embedding w/ header=false

* Fix failed tests
Copy link
Member

@npretto npretto left a comment

Choose a reason for hiding this comment

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

Tested locally
commented the wrong one 😅

WiNloSt added a commit that referenced this pull request Mar 18, 2024
…er `header=false` (#39007) (#40173)

* Remove full-app to a newer term

* Fix dashboard not loading in interactive embedding with header=false
header=false

* Fix can't choose dashboard tabs in interactive embedding w/ header=false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Automatically create PR on current release branch on merge .Team/Embedding
Projects
None yet
4 participants