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 lsq645599166 requested a review from a team as a code owner October 15, 2024 10:47
@Heenawter Heenawter self-requested a review October 15, 2024 14:52
@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
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 <[email protected]>
Co-authored-by: Hannah Mudge <[email protected]>
(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) 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 <[email protected]>
Co-authored-by: Hannah Mudge <[email protected]>
(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":"[email protected]"},"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
<[email protected]>\r\nCo-authored-by: Hannah
Mudge
<[email protected]>","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
<[email protected]>\r\nCo-authored-by: Hannah
Mudge
<[email protected]>","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
<[email protected]>\r\nCo-authored-by: Hannah
Mudge
<[email protected]>","sha":"72c0b81994d28f3983c2d21d662bd025653054fa"}}]}]
BACKPORT-->

Co-authored-by: Henry Liu <[email protected]>
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":"[email protected]"},"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
<[email protected]>\r\nCo-authored-by: Hannah
Mudge
<[email protected]>","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
<[email protected]>\r\nCo-authored-by: Hannah
Mudge
<[email protected]>","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
<[email protected]>\r\nCo-authored-by: Hannah
Mudge
<[email protected]>","sha":"72c0b81994d28f3983c2d21d662bd025653054fa"}}]}]
BACKPORT-->

Co-authored-by: Henry Liu <[email protected]>
nreese added a commit that referenced this pull request Jan 20, 2025
Fixes #205794

#196275 removed `setIsVisible`
because https://github.com/elastic/kibana/pull/196275/files#r1811034594.
I could not reproduce the top nav disappearing and reappearing issue.

---------

Co-authored-by: Elastic Machine <[email protected]>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 20, 2025
Fixes elastic#205794

elastic#196275 removed `setIsVisible`
because https://github.com/elastic/kibana/pull/196275/files#r1811034594.
I could not reproduce the top nav disappearing and reappearing issue.

---------

Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 583b852)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 20, 2025
Fixes elastic#205794

elastic#196275 removed `setIsVisible`
because https://github.com/elastic/kibana/pull/196275/files#r1811034594.
I could not reproduce the top nav disappearing and reappearing issue.

---------

Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 583b852)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jan 20, 2025
Fixes elastic#205794

elastic#196275 removed `setIsVisible`
because https://github.com/elastic/kibana/pull/196275/files#r1811034594.
I could not reproduce the top nav disappearing and reappearing issue.

---------

Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 583b852)
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Jan 21, 2025
Fixes elastic#205794

elastic#196275 removed `setIsVisible`
because https://github.com/elastic/kibana/pull/196275/files#r1811034594.
I could not reproduce the top nav disappearing and reappearing issue.

---------

Co-authored-by: Elastic Machine <[email protected]>
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
Fixes elastic#205794

elastic#196275 removed `setIsVisible`
because https://github.com/elastic/kibana/pull/196275/files#r1811034594.
I could not reproduce the top nav disappearing and reappearing issue.

---------

Co-authored-by: Elastic Machine <[email protected]>
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