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 Unlink from Library / Save to Library for Maximized Panel #150338

Merged
merged 2 commits into from Feb 7, 2023

Conversation

dmprieto
Copy link
Contributor

@dmprieto dmprieto commented Feb 6, 2023

Closes #139915

Summary

As suggested in the issue comments #139915 (comment), in this PR I added some logic after linking/unlinking to the library to set the expanded visualization id to undefined.

After testing this changes in a local development environment, the visualization is no longer disappearing from the dashboard after linking/unlinking to the library

… for maximized panels elastic#139915

- As stated in the issue comments, after linking/unlinking to the library, the expanded panel id is reset to undefined
@dmprieto dmprieto requested a review from a team as a code owner February 6, 2023 15:11
@kibanamachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Feb 6, 2023

💚 CLA has been signed

@Heenawter Heenawter added Feature:Dashboard Dashboard related features release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Feb 6, 2023
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@cqliu1 cqliu1 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this bug! I pulled down the changes locally, and it works great. This fixes the issue of the disappearing panel on link/unlink from library while the panel is maximized.

Feb-06-2023 11-31-10

@cqliu1 cqliu1 added loe:medium Medium Level of Effort impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. v8.7.0 labels Feb 6, 2023
@cqliu1
Copy link
Contributor

cqliu1 commented Feb 6, 2023

buildkite test this

@cqliu1 cqliu1 enabled auto-merge (squash) February 6, 2023 20:14
@cqliu1 cqliu1 changed the title [Dashboard] Unlink from library / Save to library actions do not work… [Dashboard] Fix Unlink from Library / Save to Library for Maximized Panel Feb 6, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
dashboard 364.6KB 364.7KB +124.0B

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

@cqliu1
Copy link
Contributor

cqliu1 commented Feb 7, 2023

@elasticmachine, run elasticsearch-ci/docs

@cqliu1 cqliu1 merged commit c71725e into elastic:main Feb 7, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 7, 2023
darnautov pushed a commit to darnautov/kibana that referenced this pull request Feb 7, 2023
elastic#150338)

… for maximized panels elastic#139915

## Summary
As suggested in the issue comments
elastic#139915 (comment),
in this PR I added some logic after linking/unlinking to the library to
set the expanded visualization id to undefined.

After testing this changes in a local development environment, the
visualization is no longer disappearing from the dashboard after
linking/unlinking to the library
cqliu1 added a commit that referenced this pull request Feb 7, 2023
## Summary

Follow up to #150338.
Closes #150460.

This updates the expanded panel to the newly generated ID when
linking/unlinking a panel from the library to keep the panel maximized.



https://user-images.githubusercontent.com/1697105/217161308-3f953f6e-7a1b-4baa-a243-f9636f04a81d.mp4

### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature
release.

When forming the risk matrix, consider some of the following examples
and how they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces—unexpected behavior in non-default Kibana Space.
| Low | High | Integration tests will verify that all features are still
supported in non-default Kibana Space and when user switches between
spaces. |
| Multiple nodes—Elasticsearch polling might have race conditions
when multiple Kibana nodes are polling for the same tasks. | High | Low
| Tasks are idempotent, so executing them multiple times will not result
in logical error, but will degrade performance. To test for this case we
add plenty of unit tests around this logic and document manual testing
procedure. |
| Code should gracefully handle cases when feature X or plugin Y are
disabled. | Medium | High | Unit tests will verify that any feature flag
or plugin combination still results in our service operational. |
| [See more potential risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
benakansara pushed a commit to benakansara/kibana that referenced this pull request Feb 7, 2023
elastic#150338)

… for maximized panels elastic#139915

## Summary
As suggested in the issue comments
elastic#139915 (comment),
in this PR I added some logic after linking/unlinking to the library to
set the expanded visualization id to undefined.

After testing this changes in a local development environment, the
visualization is no longer disappearing from the dashboard after
linking/unlinking to the library
benakansara pushed a commit to benakansara/kibana that referenced this pull request Feb 7, 2023
…ic#150405)

## Summary

Follow up to elastic#150338.
Closes elastic#150460.

This updates the expanded panel to the newly generated ID when
linking/unlinking a panel from the library to keep the panel maximized.



https://user-images.githubusercontent.com/1697105/217161308-3f953f6e-7a1b-4baa-a243-f9636f04a81d.mp4

### Checklist

Delete any items that are not applicable to this PR.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature
release.

When forming the risk matrix, consider some of the following examples
and how they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |

|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces—unexpected behavior in non-default Kibana Space.
| Low | High | Integration tests will verify that all features are still
supported in non-default Kibana Space and when user switches between
spaces. |
| Multiple nodes—Elasticsearch polling might have race conditions
when multiple Kibana nodes are polling for the same tasks. | High | Low
| Tasks are idempotent, so executing them multiple times will not result
in logical error, but will degrade performance. To test for this case we
add plenty of unit tests around this logic and document manual testing
procedure. |
| Code should gracefully handle cases when feature X or plugin Y are
disabled. | Medium | High | Unit tests will verify that any feature flag
or plugin combination still results in our service operational. |
| [See more potential risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting 💝community Feature:Dashboard Dashboard related features impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Unlink from library / Save to library actions do not work for maximized panels
6 participants