Keeps preview across different tabs #38271
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WiNloSt
changed the title
|
WiNloSt
force-pushed
the
35961-keep-preview-across-different-tabs
branch
from
95de2dc
to
22bc55f
Compare
WiNloSt
commented
Feb 1, 2024
| @@ -224,7 +227,6 @@ export const DashboardControls = ComposedComponent => | |||
| hasNightModeToggle={this.state.theme !== "transparent"} | |||
| setRefreshElapsedHook={this.setRefreshElapsedHook} | |||
| loadDashboardParams={this.loadDashboardParams} | |||
| updateDashboardParams={this.updateDashboardParams} | |||
There was a problem hiding this comment.
this was unused. You can verify by using a text search of updateDashboardParams, and no file except this file uses this function.
| @@ -423,6 +426,94 @@ describe("scenarios > embedding > dashboard parameters with defaults", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| describeEE("scenarios > embedding > dashboard appearance", () => { | |||
There was a problem hiding this comment.
Add a new test to ensure:
- Changing between parameters and appearance tabs doesn't rerender the preview
- Changing anything under appearance tab doesn't rerender the preview
| @@ -16,7 +16,7 @@ import { | |||
|
|
|||
| const { ORDERS, PRODUCTS } = SAMPLE_DATABASE; | |||
|
|
|||
| describe("scenarios > embedding > questions ", () => { | |||
| @@ -142,32 +143,3 @@ describe("issue 37914", () => { | |||
| }); | |||
| }); | |||
| }); | |||
|
|
|||
| // Code grabbed from https://www.cypress.io/blog/2020/02/12/working-with-iframes-in-cypress | |||
| const getIframeDocument = () => { | |||
There was a problem hiding this comment.
We seem to already have an existing util which does exactly this. So I'm removing a one-off util.
| @@ -49,13 +49,11 @@ describeEE("issue 26988", () => { | |||
| .click(); | |||
| popover().findByText("Oswald").click(); | |||
|
|
|||
| cy.wait("@dashboard"); | |||
There was a problem hiding this comment.
Since we're not rerendering the embed preview when changing appearance settings anymore. There would be only one @dashboard call in this test.
| onChangePane={setActivePane} | ||
| onChangeDisplayOptions={setDisplayOptions} | ||
| /> | ||
| <Tabs.Panel value={activeTab}> |
There was a problem hiding this comment.
This is a bit hacky, but essentially, I need the preview to stay in the same component, otherwise, it will unmount and remount when changing between tabs because we have 3 parents (Tabs.Panel) previously. Now it's reduced to just 1.
There was a problem hiding this comment.
It took me a bit to understand why this is working lol, even with the context of the pr and your comments. Should we add a comment in the code explaining why we're doing this? Otherwise it could be very easy to think "why are they doing this??" when reading the code :D
There was a problem hiding this comment.
Yeah, good point, the comment should go into the code.
There was a problem hiding this comment.
Added a comment in 2b8172c
| onChangeDisplayOptions={setDisplayOptions} | ||
| /> | ||
| <Tabs.Panel value={activeTab}> | ||
| {activeTab === TABS.Overview ? ( |
There was a problem hiding this comment.
Ternary isn't the best looking thing here, but I need every element to be in the same bracket, otherwise the iframe preview will rerender.
| /> | ||
| <Tabs.Panel value={activeTab}> | ||
| {activeTab === TABS.Overview ? ( | ||
| <OverviewSettings |
There was a problem hiding this comment.
I wish I could keep the preview state even when navigating to the Overview tab, but it's a bit hard to extract the content inside it here, so we only keep the embedding state when switching between Parameters and Appearance tabs.
| } | ||
| previewSlot={ | ||
| <> | ||
| <PreviewModeSelector |
There was a problem hiding this comment.
This is moved from both ParametersSettings and AppearanceSettings, the structures are similar that's why React doesn't unmount the preview when changing between these 2 tabs.
| @@ -29,12 +28,11 @@ function getSignedToken( | |||
| }); | |||
| } | |||
|
|
|||
| export function getSignedPreviewUrl( | |||
| export function getSignedPreviewUrlWithoutHash( | |||
There was a problem hiding this comment.
This is only used in frontend/src/metabase/public/components/EmbedModal/StaticEmbedSetupPane/StaticEmbedSetupPane.tsx so it's safe to modify the return value shape. I removed the hash part because I want the URL part before the hash to stay the same when only the hash is changed.
With the previous implementation, even when the hash is changed, the whole URL will be changed too.
npretto
approved these changes
Feb 1, 2024
| cy.log("Assert dashboard title"); | ||
| getIframeBody().findByText(dashboardDetails.name).should("exist"); | ||
| // We're getting an input element which is 0x0 in size | ||
| cy.findByLabelText("Dashboard title").click({ force: true }); |
There was a problem hiding this comment.
Instead of finding the element with that label, can we click the label?
There was a problem hiding this comment.
That's a good point. Do you have an example where we click the labels? I looked up testing-library, and they could only get the input using various matchers e.g. byPlaceholderText, byLabelText, but to find labels I could use findByText which is less specific, or findByRole with a lot of hoops. I'd rather use findByLabelText with force: true because I think it's still more readable.
What do you think?
There was a problem hiding this comment.
Yeah that's ok, I tried with https://testing-playground.com/ and it only prompted by findByText
| onChangePane={setActivePane} | ||
| onChangeDisplayOptions={setDisplayOptions} | ||
| /> | ||
| <Tabs.Panel value={activeTab}> |
There was a problem hiding this comment.
It took me a bit to understand why this is working lol, even with the context of the pr and your comments. Should we add a comment in the code explaining why we're doing this? Otherwise it could be very easy to think "why are they doing this??" when reading the code :D
| @@ -107,19 +113,17 @@ export const StaticEmbedSetupPane = ({ | |||
| embeddingParams, | |||
| }); | |||
|
|
|||
| const iframeUrl = useMemo( | |||
| const iframeUrlWithoutHash = useMemo( | |||
There was a problem hiding this comment.
Isn't this only used in iframeUrl which also uses the hash params?
WiNloSt
commented
Feb 2, 2024
| type: "string/contains", | ||
| }; | ||
|
|
||
| const dashboardDetails = { |
There was a problem hiding this comment.
@npretto I added this filter, since I found out today that having a parameter will cause the appearance page preview to rerender every time we change any settings
| @@ -52,7 +53,9 @@ const dashboardDetails = { | |||
|
|
|||
| describe("issue 37914", () => { | |||
| beforeEach(() => { | |||
| cy.intercept("GET", "/api/embed/dashboard/**").as("getEmbed"); | |||
| cy.intercept("GET", "/api/preview_embed/dashboard/**").as( | |||
There was a problem hiding this comment.
The preview didn't seem to work in Cypress before. I've fixed that in frontend/src/metabase/lib/dom.js
| @@ -40,7 +42,7 @@ describeEE("issue 26988", () => { | |||
| acceptTerms: false, | |||
| }); | |||
|
|
|||
| cy.wait("@dashboard"); | |||
| cy.wait("@previewDashboard"); | |||
There was a problem hiding this comment.
The preview didn't seem to work in Cypress before. I've fixed that in frontend/src/metabase/lib/dom.js
| @@ -26,7 +26,7 @@ window.METABASE = true; | |||
| // used for detecting if we're previewing an embed | |||
| export const IFRAMED_IN_SELF = (function () { | |||
| try { | |||
| return window.self !== window.top && window.top.METABASE; | |||
| return window.self !== window.parent && window.parent.METABASE; | |||
There was a problem hiding this comment.
IFRAMED_IN_SELF was only used in a few places. And all of them relate to how we're rendering the embed preview.
This was failing in Cypress because there are 3 frames in Cypress
- Cypress
- Metabase Iframe
- Embed preview Iframe
- Metabase Iframe
But in our application, there are only 2 frames
- Metabase
- Embed preview Iframe
For this particular code, when running in Cypress window.parent will get Cypress frame, so the function will return false, which was wrong.
Changing it to window.parent, makes sure it returns the immediate parent frame which will be Metabase frame in both cases.
This was referenced May 8, 2024
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of a milestone 6 of
Epic: #35961
Description
Make sure embed preview doesn't reload when changing between tabs/preview toggle/appearance settings
Please note that when going to the overview tab, the preview will be unmounted, so going to the preview again after visiting the overview tab will cause the iframe to reload. This is a compromise for the current codebase. I need to put a lot more work on the overview tab to make that happen, so it didn't happen.
How to verify
Tests both dashboards and questions the same way.
Demo
https://www.loom.com/share/f700c55d53634f5b974bba30f4f55867
Checklist