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

Simplify workflow for dashboard copy creation in both view and edit interaction modes #180938

Merged
merged 26 commits into from
May 29, 2024

Conversation

eokoneyo
Copy link
Contributor

@eokoneyo eokoneyo commented Apr 16, 2024

Summary

Closes #161047

  • Removes the save as top nav menu button
  • Also renames nav menu item clone to duplicate and make it available in edit mode.
  • The save dashboard modal no longer displays and open to save the dashboard in context as new, given that we've chosen to explicitly create a copy of the dashboard in context when either of the the duplicate or saveas menu option is selected.
  • includes bug fix for an issue where clicking the dashboard modal scrolled the user to the content bottom, see Simplify workflow for dashboard copy creation in both view and edit interaction modes #180938 (comment)

Before

View mode

Screenshot 2024-04-16 at 15 59 10

Edit mode

Screenshot 2024-04-16 at 15 59 00

After

Managed Dashboard

Screen.Recording.2024-05-15.at.10.39.21.mov

View mode

Screen.Recording.2024-05-15.at.10.42.09.mov

Edit mode

Screen.Recording.2024-05-15.at.10.40.59.mov

@eokoneyo eokoneyo added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas Project:Dashboard Usability Related to the Dashboard Usability initiative labels Apr 16, 2024
@eokoneyo eokoneyo self-assigned this Apr 16, 2024
@cqliu1 cqliu1 self-requested a review April 16, 2024 15:24
@cqliu1 cqliu1 added Feature:Dashboard Dashboard related features v8.14.0 release_note:enhancement 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 Apr 16, 2024
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.

Code LGTM. I ran this locally and have just a few nits on the UI side:

Can we move the Switch to view mode button left of the Settings button in edit mode? That way the Share button doesn't jump around when switching between view and edit mode.

Apr-16-2024 08-48-21

Also, I noticed when switching to the edit mode, the Duplicate button flashes and appears to disable for a second. The Reset button also does the same thing where it's enabled before it's disabled when switching back to view mode. Not a blocker for this PR if it isn't an easy fix, and we can file a issue to resolve later.

@eokoneyo
Copy link
Contributor Author

Code LGTM. I ran this locally and have just a few nits on the UI side:

Can we move the Switch to view mode button left of the Settings button in edit mode? That way the Share button doesn't jump around when switching between view and edit mode.

Apr-16-2024 08-48-21

Also, I noticed when switching to the edit mode, the Duplicate button flashes and appears to disable for a second. The Reset button also does the same thing where it's enabled before it's disabled when switching back to view mode. Not a blocker for this PR if it isn't an easy fix, and we can file a issue to resolve later.

@cqliu1 so there's already an existing grouping for rendering nav menu items, that been said I think I think modifying it with would be out of the scope of this PR. I did make some change still so that the existing grouping is preserved and keeps us close to how the nav item has been. Let me know what you think?

ScreenRecording2024-04-16at17 19 21-ezgif com-video-to-gif-converter

@eokoneyo eokoneyo marked this pull request as ready for review April 16, 2024 20:58
@eokoneyo eokoneyo requested a review from a team as a code owner April 16, 2024 20:58
@elasticmachine
Copy link
Contributor

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

@eokoneyo
Copy link
Contributor Author

/ci

@cqliu1
Copy link
Contributor

cqliu1 commented Apr 16, 2024

@cqliu1 so there's already an existing grouping for rendering nav menu items, that been said I think I think modifying it with would be out of the scope of this PR. I did make some change still so that the existing grouping is preserved and keeps us close to how the nav item has been. Let me know what you think?

ScreenRecording2024-04-16at17 19 21-ezgif com-video-to-gif-converter

That works for now 👍

@eokoneyo eokoneyo added v8.15.0 and removed v8.14.0 labels Apr 17, 2024
@cqliu1 cqliu1 self-requested a review April 24, 2024 15:55
@eokoneyo eokoneyo marked this pull request as draft May 3, 2024 14:39
@eokoneyo
Copy link
Contributor Author

eokoneyo commented May 3, 2024

Reverting to draft given the work scope changed. Will transition when ready for review

@eokoneyo
Copy link
Contributor Author

eokoneyo commented May 3, 2024

/ci

@eokoneyo
Copy link
Contributor Author

eokoneyo commented May 9, 2024

/ci

@eokoneyo
Copy link
Contributor Author

/ci

@eokoneyo
Copy link
Contributor Author

... Only one nit: I'd prefer if this description for the Store time with dashboard setting was a tooltip instead, but I'll approve to unblock.

Good callout with the moving the store time with dashboard description into a tooltip, it looks so much better!

Screenshot 2024-05-29 at 10 07 03

@kibana-ci
Copy link
Collaborator

kibana-ci commented May 29, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

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
savedObjects 120 121 +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 495.1KB 494.9KB -206.0B

Page load bundle

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

id before after diff
savedObjects 25.3KB 25.5KB +152.0B
Unknown metric groups

API count

id before after diff
savedObjects 131 132 +1

ESLint disabled line counts

id before after diff
savedObjects 0 1 +1

Total ESLint disabled count

id before after diff
savedObjects 0 1 +1

History

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

cc @eokoneyo

@eokoneyo eokoneyo merged commit 690690e into elastic:main May 29, 2024
20 checks passed
@eokoneyo eokoneyo deleted the fix/resolve-161047 branch May 29, 2024 09:46
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label May 29, 2024
rshen91 pushed a commit to rshen91/kibana that referenced this pull request May 30, 2024
…nteraction modes (elastic#180938)

## Summary

Closes elastic#161047

- Removes the `save as` top nav menu button
- Also renames nav menu item `clone` to `duplicate` and make it
available in edit mode.
- The save dashboard modal no longer displays and open to save the
dashboard in context as new, given that we've chosen to explicitly
create a copy of the dashboard in context when either of the the
`duplicate` or `saveas` menu option is selected.
- includes bug fix for an issue where clicking the dashboard modal
scrolled the user to the content bottom, see
elastic#180938 (comment)

## Before
### View mode
<img width="1728" alt="Screenshot 2024-04-16 at 15 59 10"
src="https://github.com/elastic/kibana/assets/7893459/48dc4565-1f75-4f46-839c-8d76f4fedefe">

### Edit mode
<img width="1725" alt="Screenshot 2024-04-16 at 15 59 00"
src="https://github.com/elastic/kibana/assets/7893459/1ac743ac-33b4-4f68-ab59-ad19ab58fa1c">

## After

#### Managed Dashboard

https://github.com/elastic/kibana/assets/7893459/5072a501-8d16-4f25-9575-6f11fed6e580

#### View mode

https://github.com/elastic/kibana/assets/7893459/610d0952-97f0-46b8-a0ea-1546a799d387

#### Edit mode

https://github.com/elastic/kibana/assets/7893459/4f596c07-7bd1-4c5a-9131-0c78731cb113



<!-- ### 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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] 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&mdash;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&mdash;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)
-->

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
eokoneyo added a commit that referenced this pull request Jun 8, 2024
…#184777)

## Summary

Follow up to #180938.

On `Save as`, the suggested title should always be a unique title. I've
added a check for duplicate titles when generating a suggested new
dashboard title. If the dashboards `Dashboard A`, `Dashboard A (1)`, and
`Dashboard A (2)` already exist, the next suggested dashboard title will
be `Dashboard A (3)`. This way the user doesn't end up hitting the
duplicate title warning if they want to quickly clone a dashboard
without updating the title.

![Jun-04-2024
12-27-59](https://github.com/elastic/kibana/assets/1697105/b1f1ae1a-87d3-4dcd-80cd-395bd6ee9800)

### 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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] 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&mdash;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&mdash;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)

---------

Co-authored-by: Eyo Okon Eyo <eyo.eyo@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
eokoneyo added a commit to eokoneyo/kibana that referenced this pull request Jun 13, 2024
…elastic#184777)

## Summary

Follow up to elastic#180938.

On `Save as`, the suggested title should always be a unique title. I've
added a check for duplicate titles when generating a suggested new
dashboard title. If the dashboards `Dashboard A`, `Dashboard A (1)`, and
`Dashboard A (2)` already exist, the next suggested dashboard title will
be `Dashboard A (3)`. This way the user doesn't end up hitting the
duplicate title warning if they want to quickly clone a dashboard
without updating the title.

![Jun-04-2024
12-27-59](https://github.com/elastic/kibana/assets/1697105/b1f1ae1a-87d3-4dcd-80cd-395bd6ee9800)

### 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
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] 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&mdash;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&mdash;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)

---------

Co-authored-by: Eyo Okon Eyo <eyo.eyo@elastic.co>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
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 ci:cloud-deploy Create or update a Cloud deployment Feature:Dashboard Dashboard related features 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 Project:Dashboard Usability Related to the Dashboard Usability initiative release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Simplify Save as workflow