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] Fix share dashboard iframe embed code incorrectly #194366

Merged
merged 5 commits into from
Oct 3, 2024

Conversation

viajes7
Copy link
Contributor

@viajes7 viajes7 commented Sep 30, 2024

Summary

Fixes #192980

Maybe related pr: #179037

After fix it, having an URL that is like this:

?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true

Screenshot
image

@viajes7 viajes7 requested a review from a team as a code owner September 30, 2024 02:58
@tsullivan
Copy link
Member

Hi @viajes7 there is a duplicate issue of the one you picked up: #193447. Since that issue was triaged to my team sooner, I already have a fix of that one which is in review: #194185. Sorry about this mixup.

How do you feel about closing this PR and sticking to the one I have? I prefer my PR since it also adds tests to the fix.

@viajes7 viajes7 closed this Sep 30, 2024
@viajes7 viajes7 reopened this Oct 1, 2024
@viajes7
Copy link
Contributor Author

viajes7 commented Oct 1, 2024

Hi, @tsullivan , after reviewed the pr #194185 , I think my modification is still advisable.

In make_iframe_tag.ts, two things were done, iframe tag was created, and embed parameter was added.

Consider clean code, this component is not suitable for Single Responsibility Principle. Maybe update it in updateUrlParams method is more suitable?


On the other hand, the embed_content.tsx currently does not have a complete unit test. I think this can be optimized later.

And I find that the isEmbedded prop had no actual effect. So I pushed another commit to remove it.

Just my opinion, waiting for your news. 😄

@tsullivan
Copy link
Member

On the other hand, the embed_content.tsx currently does not have a complete unit test. I think this can be optimized later.

@viajes7 If you can add a unit test for embed_content in this PR that covers the issue being fixed, then I would agree that we should deliver your version of this fix.

@tsullivan
Copy link
Member

/ci

@viajes7
Copy link
Contributor Author

viajes7 commented Oct 2, 2024

@elasticmachine merge upstream

@viajes7
Copy link
Contributor Author

viajes7 commented Oct 2, 2024

@tsullivan Hi, Added some test cases for embed shareUrl. Please have a look, thanks.

@tsullivan tsullivan added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label Oct 2, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your contribution, and for adding the tests!

@tsullivan
Copy link
Member

/ci

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
share 56.9KB 57.0KB +69.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan
Copy link
Member

@elasticmachine run docs-build

@tsullivan tsullivan enabled auto-merge (squash) October 3, 2024 00:02
@tsullivan tsullivan merged commit e4ff3bb into elastic:main Oct 3, 2024
21 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.15, 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 3, 2024
…c#194366)

## Summary

Fixes [elastic#192980](elastic#193447)

Maybe related pr: elastic#179037

After fix it, having an URL that is like this:
```
?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true
```

**Screenshot**
<img width="1918" alt="image"
src="https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e">

---------

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Tim Sullivan <[email protected]>
(cherry picked from commit e4ff3bb)
@kibanamachine
Copy link
Contributor

💔 Some backports could not be created

Status Branch Result
8.15 Backport failed because of merge conflicts
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 194366

Questions ?

Please refer to the Backport tool documentation

@tsullivan
Copy link
Member

💚 All backports created successfully

Status Branch Result
8.15

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 3, 2024
…194366) (#194773)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Dashboard] Fix share dashboard iframe embed code incorrectly
(#194366)](#194366)

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

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

<!--BACKPORT [{"author":{"name":"Jusheng
Huang","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-03T00:24:46Z","message":"[Dashboard]
Fix share dashboard iframe embed code incorrectly (#194366)\n\n##
Summary\r\n\r\nFixes
[#192980](https://github.com/elastic/kibana/issues/193447)\r\n\r\nMaybe
related pr: #179037 \r\n\r\nAfter fix it, having an URL that is like
this:\r\n```\r\n?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true\r\n```\r\n\r\n**Screenshot**\r\n<img
width=\"1918\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Tim
Sullivan
<[email protected]>","sha":"e4ff3bb44874f77a63c24de998fde2f6d7ac9404","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","💝community","v9.0.0","Team:SharedUX","v8.16.0","backport:version","v8.15.3"],"title":"[Dashboard]
Fix share dashboard iframe embed code
incorrectly","number":194366,"url":"https://github.com/elastic/kibana/pull/194366","mergeCommit":{"message":"[Dashboard]
Fix share dashboard iframe embed code incorrectly (#194366)\n\n##
Summary\r\n\r\nFixes
[#192980](https://github.com/elastic/kibana/issues/193447)\r\n\r\nMaybe
related pr: #179037 \r\n\r\nAfter fix it, having an URL that is like
this:\r\n```\r\n?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true\r\n```\r\n\r\n**Screenshot**\r\n<img
width=\"1918\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Tim
Sullivan
<[email protected]>","sha":"e4ff3bb44874f77a63c24de998fde2f6d7ac9404"}},"sourceBranch":"main","suggestedTargetBranches":["8.x","8.15"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194366","number":194366,"mergeCommit":{"message":"[Dashboard]
Fix share dashboard iframe embed code incorrectly (#194366)\n\n##
Summary\r\n\r\nFixes
[#192980](https://github.com/elastic/kibana/issues/193447)\r\n\r\nMaybe
related pr: #179037 \r\n\r\nAfter fix it, having an URL that is like
this:\r\n```\r\n?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true\r\n```\r\n\r\n**Screenshot**\r\n<img
width=\"1918\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Tim
Sullivan
<[email protected]>","sha":"e4ff3bb44874f77a63c24de998fde2f6d7ac9404"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.15","label":"v8.15.3","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Jusheng Huang <[email protected]>
tsullivan pushed a commit to tsullivan/kibana that referenced this pull request Oct 3, 2024
…c#194366)

## Summary

Fixes [elastic#192980](elastic#193447)

Maybe related pr: elastic#179037

After fix it, having an URL that is like this:
```
?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true
```

**Screenshot**
<img width="1918" alt="image"
src="https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e">

---------

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Tim Sullivan <[email protected]>
(cherry picked from commit e4ff3bb)

# Conflicts:
#	src/plugins/share/public/components/share_tabs.test.tsx
#	src/plugins/share/public/services/share_menu_manager.tsx
@kibanamachine
Copy link
Contributor

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 4, 2024
tsullivan added a commit that referenced this pull request Oct 4, 2024
…194366) (#194774)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[Dashboard] Fix share dashboard iframe embed code incorrectly
(#194366)](#194366)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Jusheng
Huang","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-03T00:24:46Z","message":"[Dashboard]
Fix share dashboard iframe embed code incorrectly (#194366)\n\n##
Summary\r\n\r\nFixes
[#192980](https://github.com/elastic/kibana/issues/193447)\r\n\r\nMaybe
related pr: #179037 \r\n\r\nAfter fix it, having an URL that is like
this:\r\n```\r\n?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true\r\n```\r\n\r\n**Screenshot**\r\n<img
width=\"1918\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Tim
Sullivan
<[email protected]>","sha":"e4ff3bb44874f77a63c24de998fde2f6d7ac9404","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","💝community","v9.0.0","Team:SharedUX","v8.16.0","backport:version","v8.15.3"],"number":194366,"url":"https://github.com/elastic/kibana/pull/194366","mergeCommit":{"message":"[Dashboard]
Fix share dashboard iframe embed code incorrectly (#194366)\n\n##
Summary\r\n\r\nFixes
[#192980](https://github.com/elastic/kibana/issues/193447)\r\n\r\nMaybe
related pr: #179037 \r\n\r\nAfter fix it, having an URL that is like
this:\r\n```\r\n?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true\r\n```\r\n\r\n**Screenshot**\r\n<img
width=\"1918\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Tim
Sullivan
<[email protected]>","sha":"e4ff3bb44874f77a63c24de998fde2f6d7ac9404"}},"sourceBranch":"main","suggestedTargetBranches":["8.15"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194366","number":194366,"mergeCommit":{"message":"[Dashboard]
Fix share dashboard iframe embed code incorrectly (#194366)\n\n##
Summary\r\n\r\nFixes
[#192980](https://github.com/elastic/kibana/issues/193447)\r\n\r\nMaybe
related pr: #179037 \r\n\r\nAfter fix it, having an URL that is like
this:\r\n```\r\n?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true\r\n```\r\n\r\n**Screenshot**\r\n<img
width=\"1918\"
alt=\"image\"\r\nsrc=\"https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Elastic Machine
<[email protected]>\r\nCo-authored-by: Tim
Sullivan
<[email protected]>","sha":"e4ff3bb44874f77a63c24de998fde2f6d7ac9404"}},{"branch":"8.x","label":"v8.16.0","labelRegex":"^v8.16.0$","isSourceBranch":false,"url":"https://github.com/elastic/kibana/pull/194773","number":194773,"state":"OPEN"},{"branch":"8.15","label":"v8.15.3","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

---------

Co-authored-by: Jusheng Huang <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 4, 2024
tiansivive pushed a commit to tiansivive/kibana that referenced this pull request Oct 7, 2024
…c#194366)

## Summary

Fixes [elastic#192980](elastic#193447)

Maybe related pr: elastic#179037 

After fix it, having an URL that is like this:
```
?embed=true&_g=(refreshInterval%3A(pause%3A!t%2Cvalue%3A60000)%2Ctime%3A(from%3Anow-15m%2Cto%3Anow))&hide-filter-bar=true
```

**Screenshot**
<img width="1918" alt="image"
src="https://github.com/user-attachments/assets/de069fdb-895e-479a-b868-891bbb55594e">

---------

Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Tim Sullivan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels 💝community release_note:fix Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) v8.15.3 v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

embed=true missing from embedding dashboard URLs (Share > Embed > Copy Link)
5 participants