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

[Dashboard] show fullscreen mode when url has fullScreenMode param #196275

Merged
merged 7 commits into from
Oct 24, 2024

Conversation

lsq645599166
Copy link
Contributor

@lsq645599166 lsq645599166 commented Oct 15, 2024

Summary

Closes #174561

Show fullscreen mode when url has fullScreenMode param &_a=(fullScreenMode:!t)

Screenshot

20241015-184503

@lsq645599166
Copy link
Contributor Author

@elasticmachine merge upstream

@lsq645599166
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

@nreese I am defaulting to your opinion here :) I think that fullscreenMode belongs under InitialComponentState, but our current URL logic is tied to the old DashboardContainerInput type. What is the best way to handle this, do you think? Should we just keep it under DashboardContainerInput and you can address it in your refactor?

@lsq645599166
Copy link
Contributor Author

@nreese I am defaulting to your opinion here :) I think that fullscreenMode belongs under InitialComponentState, but our current URL logic is tied to the old DashboardContainerInput type. What is the best way to handle this, do you think? Should we just keep it under DashboardContainerInput and you can address it in your refactor?

This involves a lot of parameters and attributes, so I can't give a precise answer. Maybe maintaining the status quo is a better choice.

@nreese
Copy link
Contributor

nreese commented Oct 21, 2024

@nreese I am defaulting to your opinion here :) I think that fullscreenMode belongs under InitialComponentState, but our current URL logic is tied to the old DashboardContainerInput type. What is the best way to handle this, do you think? Should we just keep it under DashboardContainerInput and you can address it in your refactor?

Putting fullscreenMode into InitialComponentState makes the most sense

@nreese
Copy link
Contributor

nreese commented Oct 21, 2024

This involves a lot of parameters and attributes, so I can't give a precise answer. Maybe maintaining the status quo is a better choice.

How about just putting fullScreenMode as a top level key in creationOptions, Then its value can just be read directly from the URL params instead of using loadAndRemoveDashboardState utility

See it will look something like this

const getCreationOptions = useCallback((): Promise<DashboardCreationOptions> => {
    const searchSessionIdFromURL = getSearchSessionIdFromURL(history);
    const getInitialInput = () => {
      const stateFromLocator = loadDashboardHistoryLocationState(getScopedHistory);
      const initialUrlState = loadAndRemoveDashboardState(kbnUrlStateStorage);

      // Override all state with URL + Locator input
      return {
        // State loaded from the dashboard app URL and from the locator overrides all other dashboard state.
        ...initialUrlState,
        ...stateFromLocator,

        // if print mode is active, force viewMode.PRINT
        ...(screenshotModeService.isScreenshotMode() &&
        screenshotModeService.getScreenshotContext('layout') === 'print'
          ? { viewMode: ViewMode.PRINT }
          : {}),
      };
    };

    return Promise.resolve<DashboardCreationOptions>({
      getIncomingEmbeddable: () =>
        embeddableService.getStateTransfer().getIncomingEmbeddablePackage(DASHBOARD_APP_ID, true),

      // integrations
      useSessionStorageIntegration: true,
      useUnifiedSearchIntegration: true,
      unifiedSearchSettings: {
        kbnUrlStateStorage,
      },
      useSearchSessionsIntegration: true,
      searchSessionSettings: {
        createSessionRestorationDataProvider,
        sessionIdToRestore: searchSessionIdFromURL,
        sessionIdUrlChangeObservable: getSessionURLObservable(history),
        getSearchSessionIdFromURL: () => getSearchSessionIdFromURL(history),
        removeSessionIdFromUrl: () => removeSearchSessionIdFromURL(kbnUrlStateStorage),
      },
      getInitialInput,
      validateLoadedSavedObject: validateOutcome,
      fullScreenMode: kbnUrlStateStorage.get<{ fullScreenMode?: boolean }>(
        DASHBOARD_STATE_STORAGE_KEY
      )?.fullScreenMode ?? false;
      isEmbeddedExternally: Boolean(embedSettings), // embed settings are only sent if the dashboard URL has `embed=true`
      getEmbeddableAppContext: (dashboardId) => ({
        currentAppId: DASHBOARD_APP_ID,
        getCurrentPath: () => `#${createDashboardEditUrl(dashboardId)}`,
      }),
    });
  }, [history, embedSettings, validateOutcome, getScopedHistory, kbnUrlStateStorage]);

@lsq645599166
Copy link
Contributor Author

This involves a lot of parameters and attributes, so I can't give a precise answer. Maybe maintaining the status quo is a better choice.

How about just putting fullScreenMode as a top level key in creationOptions, Then its value can just be read directly from the URL params instead of using loadAndRemoveDashboardState utility

See it will look something like this

const getCreationOptions = useCallback((): Promise<DashboardCreationOptions> => {
    const searchSessionIdFromURL = getSearchSessionIdFromURL(history);
    const getInitialInput = () => {
      const stateFromLocator = loadDashboardHistoryLocationState(getScopedHistory);
      const initialUrlState = loadAndRemoveDashboardState(kbnUrlStateStorage);

      // Override all state with URL + Locator input
      return {
        // State loaded from the dashboard app URL and from the locator overrides all other dashboard state.
        ...initialUrlState,
        ...stateFromLocator,

        // if print mode is active, force viewMode.PRINT
        ...(screenshotModeService.isScreenshotMode() &&
        screenshotModeService.getScreenshotContext('layout') === 'print'
          ? { viewMode: ViewMode.PRINT }
          : {}),
      };
    };

    return Promise.resolve<DashboardCreationOptions>({
      getIncomingEmbeddable: () =>
        embeddableService.getStateTransfer().getIncomingEmbeddablePackage(DASHBOARD_APP_ID, true),

      // integrations
      useSessionStorageIntegration: true,
      useUnifiedSearchIntegration: true,
      unifiedSearchSettings: {
        kbnUrlStateStorage,
      },
      useSearchSessionsIntegration: true,
      searchSessionSettings: {
        createSessionRestorationDataProvider,
        sessionIdToRestore: searchSessionIdFromURL,
        sessionIdUrlChangeObservable: getSessionURLObservable(history),
        getSearchSessionIdFromURL: () => getSearchSessionIdFromURL(history),
        removeSessionIdFromUrl: () => removeSearchSessionIdFromURL(kbnUrlStateStorage),
      },
      getInitialInput,
      validateLoadedSavedObject: validateOutcome,
      fullScreenMode: kbnUrlStateStorage.get<{ fullScreenMode?: boolean }>(
        DASHBOARD_STATE_STORAGE_KEY
      )?.fullScreenMode ?? false;
      isEmbeddedExternally: Boolean(embedSettings), // embed settings are only sent if the dashboard URL has `embed=true`
      getEmbeddableAppContext: (dashboardId) => ({
        currentAppId: DASHBOARD_APP_ID,
        getCurrentPath: () => `#${createDashboardEditUrl(dashboardId)}`,
      }),
    });
  }, [history, embedSettings, validateOutcome, getScopedHistory, kbnUrlStateStorage]);

Thanks, I fixed it and can you take a look

@Heenawter Heenawter added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Oct 21, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@Heenawter Heenawter added release_note:fix loe:small Small Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Oct 21, 2024
@Heenawter
Copy link
Contributor

buildkite test this

@Heenawter
Copy link
Contributor

@elasticmachine run buildkite/docs-build-pr

@Heenawter Heenawter added backport:version Backport to applied version labels v8.16.0 v8.17.0 v8.15.4 and removed v8.16.0 backport:version Backport to applied version labels v8.17.0 labels Oct 21, 2024
@Heenawter Heenawter added backport:prev-major Backport to (8.x, 8.17, 8.16, 8.15) the previous major branch and other branches in development and removed v8.15.4 labels Oct 21, 2024
@Heenawter
Copy link
Contributor

@lsq645599166 Here's a screenshot of the type errors:

image

@lsq645599166
Copy link
Contributor Author

@lsq645599166 Here's a screenshot of the type errors:

image

Thanks and fixed

@Heenawter
Copy link
Contributor

buildkite test this

@Heenawter
Copy link
Contributor

@elasticmachine run buildkite/docs-build-pr

@@ -123,13 +123,6 @@ export function InternalDashboardTopNav({
dashboardTitleRef.current?.focus();
}, [title, viewMode]);

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If keep this, the top nav will disappear and reappear like below

20241023-001416.mp4

And for embed mode, this function will ensure it works fine

private initVisibility(application: StartDeps['application']) {
// Start off the chrome service hidden if "embed" is in the hash query string.
const isEmbedded = 'embed' in parse(location.hash.slice(1), true).query;
this.isForceHidden$ = new BehaviorSubject(isEmbedded);
const appHidden$ = merge(
// For the isVisible$ logic, having no mounted app is equivalent to having a hidden app
// in the sense that the chrome UI should not be displayed until a non-chromeless app is mounting or mounted
of(true),
application.currentAppId$.pipe(
mergeMap((appId) =>
application.applications$.pipe(
map((applications) => {
return !!appId && applications.has(appId) && !!applications.get(appId)!.chromeless;
})
)
)
)
);
this.isVisible$ = combineLatest([appHidden$, this.isForceHidden$]).pipe(
map(([appHidden, forceHidden]) => !appHidden && !forceHidden),
takeUntil(this.stop$)
);
}

If there is a better way, I will fix it.

@Heenawter
Copy link
Contributor

buildkite test this

@Heenawter
Copy link
Contributor

@elasticmachine run buildkite/docs-build-pr

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
dashboard 124 125 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
dashboard 643.0KB 643.2KB +139.0B
Unknown metric groups

API count

id before after diff
dashboard 129 130 +1

History

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

Changes LGTM - code review + local testing. Thanks so much for tackling this 🙇

@Heenawter Heenawter merged commit 72c0b81 into elastic:main Oct 24, 2024
21 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.15, 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11507245328

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 24, 2024
…lastic#196275)

## Summary

Closes elastic#174561

Show fullscreen mode when url has fullScreenMode param
`&_a=(fullScreenMode:!t)`

### Screenshot

![20241015-184503](https://github.com/user-attachments/assets/fae01dcc-f081-4314-84f9-3923adc76e5b)

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
(cherry picked from commit 72c0b81)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
8.15 Backport failed because of merge conflicts
8.16
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Manual backport

To create the backport manually run:

node scripts/backport --pr 196275

Questions ?

Please refer to the Backport tool documentation

@Heenawter Heenawter added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:prev-major Backport to (8.x, 8.17, 8.16, 8.15) the previous major branch and other branches in development labels Oct 24, 2024
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11507553634

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 24, 2024
…lastic#196275)

## Summary

Closes elastic#174561

Show fullscreen mode when url has fullScreenMode param
`&_a=(fullScreenMode:!t)`

### Screenshot

![20241015-184503](https://github.com/user-attachments/assets/fae01dcc-f081-4314-84f9-3923adc76e5b)

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Hannah Mudge <Heenawter@users.noreply.github.com>
(cherry picked from commit 72c0b81)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Oct 24, 2024
…aram (#196275) (#197728)

# Backport

This will backport the following commits from `main` to `8.16`:
- [[Dashboard] show fullscreen mode when url has fullScreenMode param
(#196275)](#196275)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Henry
Liu","email":"645599166@qq.com"},"sourceCommit":{"committedDate":"2024-10-24T20:49:39Z","message":"[Dashboard]
show fullscreen mode when url has fullScreenMode param (#196275)\n\n##
Summary\r\n\r\nCloses #174561\r\n\r\nShow fullscreen mode when url has
fullScreenMode param\r\n`&_a=(fullScreenMode:!t)`\r\n\r\n###
Screenshot\r\n\r\n\r\n![20241015-184503](https://github.com/user-attachments/assets/fae01dcc-f081-4314-84f9-3923adc76e5b)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Hannah
Mudge
<Heenawter@users.noreply.github.com>","sha":"72c0b81994d28f3983c2d21d662bd025653054fa","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","loe:small","impact:medium","💝community","v9.0.0","backport:prev-major"],"title":"[Dashboard]
show fullscreen mode when url has fullScreenMode
param","number":196275,"url":"https://github.com/elastic/kibana/pull/196275","mergeCommit":{"message":"[Dashboard]
show fullscreen mode when url has fullScreenMode param (#196275)\n\n##
Summary\r\n\r\nCloses #174561\r\n\r\nShow fullscreen mode when url has
fullScreenMode param\r\n`&_a=(fullScreenMode:!t)`\r\n\r\n###
Screenshot\r\n\r\n\r\n![20241015-184503](https://github.com/user-attachments/assets/fae01dcc-f081-4314-84f9-3923adc76e5b)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Hannah
Mudge
<Heenawter@users.noreply.github.com>","sha":"72c0b81994d28f3983c2d21d662bd025653054fa"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196275","number":196275,"mergeCommit":{"message":"[Dashboard]
show fullscreen mode when url has fullScreenMode param (#196275)\n\n##
Summary\r\n\r\nCloses #174561\r\n\r\nShow fullscreen mode when url has
fullScreenMode param\r\n`&_a=(fullScreenMode:!t)`\r\n\r\n###
Screenshot\r\n\r\n\r\n![20241015-184503](https://github.com/user-attachments/assets/fae01dcc-f081-4314-84f9-3923adc76e5b)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Hannah
Mudge
<Heenawter@users.noreply.github.com>","sha":"72c0b81994d28f3983c2d21d662bd025653054fa"}}]}]
BACKPORT-->

Co-authored-by: Henry Liu <645599166@qq.com>
kibanamachine added a commit that referenced this pull request Oct 24, 2024
…ram (#196275) (#197729)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Dashboard] show fullscreen mode when url has fullScreenMode param
(#196275)](#196275)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Henry
Liu","email":"645599166@qq.com"},"sourceCommit":{"committedDate":"2024-10-24T20:49:39Z","message":"[Dashboard]
show fullscreen mode when url has fullScreenMode param (#196275)\n\n##
Summary\r\n\r\nCloses #174561\r\n\r\nShow fullscreen mode when url has
fullScreenMode param\r\n`&_a=(fullScreenMode:!t)`\r\n\r\n###
Screenshot\r\n\r\n\r\n![20241015-184503](https://github.com/user-attachments/assets/fae01dcc-f081-4314-84f9-3923adc76e5b)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Hannah
Mudge
<Heenawter@users.noreply.github.com>","sha":"72c0b81994d28f3983c2d21d662bd025653054fa","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","loe:small","impact:medium","💝community","v9.0.0","backport:prev-major"],"title":"[Dashboard]
show fullscreen mode when url has fullScreenMode
param","number":196275,"url":"https://github.com/elastic/kibana/pull/196275","mergeCommit":{"message":"[Dashboard]
show fullscreen mode when url has fullScreenMode param (#196275)\n\n##
Summary\r\n\r\nCloses #174561\r\n\r\nShow fullscreen mode when url has
fullScreenMode param\r\n`&_a=(fullScreenMode:!t)`\r\n\r\n###
Screenshot\r\n\r\n\r\n![20241015-184503](https://github.com/user-attachments/assets/fae01dcc-f081-4314-84f9-3923adc76e5b)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Hannah
Mudge
<Heenawter@users.noreply.github.com>","sha":"72c0b81994d28f3983c2d21d662bd025653054fa"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/196275","number":196275,"mergeCommit":{"message":"[Dashboard]
show fullscreen mode when url has fullScreenMode param (#196275)\n\n##
Summary\r\n\r\nCloses #174561\r\n\r\nShow fullscreen mode when url has
fullScreenMode param\r\n`&_a=(fullScreenMode:!t)`\r\n\r\n###
Screenshot\r\n\r\n\r\n![20241015-184503](https://github.com/user-attachments/assets/fae01dcc-f081-4314-84f9-3923adc76e5b)\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<elasticmachine@users.noreply.github.com>\r\nCo-authored-by: Hannah
Mudge
<Heenawter@users.noreply.github.com>","sha":"72c0b81994d28f3983c2d21d662bd025653054fa"}}]}]
BACKPORT-->

Co-authored-by: Henry Liu <645599166@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) 💝community impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.0 v8.17.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fullscreen URL param no longer works
5 participants