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 Usability] Unified dashboard settings #153862

Merged
merged 24 commits into from
Mar 31, 2023

Conversation

nickpeihl
Copy link
Member

@nickpeihl nickpeihl commented Mar 28, 2023

Summary

Adds flyout for changing individual dashboard settings such as title, description, tags, and save time with dashboard. This also moves the existing dashboard options (show panel titles, sync colors, use margins, sync cursor, and sync tooltips) into the flyout.

Fixes #144532

Flaky test runner

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@nickpeihl nickpeihl added release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.8.0 Feature:Dashboard Dashboard related features labels Mar 28, 2023
@nickpeihl nickpeihl marked this pull request as ready for review March 28, 2023 18:48
@nickpeihl nickpeihl requested review from a team as code owners March 28, 2023 18:48
@elasticmachine
Copy link
Contributor

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

} = this.getReduxEmbeddableTools();

// TODO Move this action into DashboardContainer.openOverlay
dispatch(setHasOverlays(true));
Copy link
Member Author

Choose a reason for hiding this comment

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

This will disable the Quick save button while the flyout is open. We could move this into DashboardContainer.openOverlay, but we would need to confirm that other flyouts should disable the save button.

@nickpeihl nickpeihl added loe:large Large Level of Effort impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Mar 28, 2023
@nreese nreese self-requested a review March 28, 2023 21:44
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

This looks great and is a huge step forward in usability. Great looking functional tests as well.

@@ -363,7 +363,7 @@ Apply a set of design options to the entire dashboard.

. If you're in view mode, click *Edit* in the toolbar.

. In the toolbar, *Options*, then use the following options:
. In the toolbar, click *Settings*, to open the *Dashboard settings* flyout, then use the following options:
Copy link
Contributor

Choose a reason for hiding this comment

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

Something for a follow up PR - rewrite "Apply design options" section into something more appropriate for the new UI, such as "Dashboard settings". Then expand to include all dashboard options

description: i18n.translate('dashboard.topNave.optionsConfigDescription', {
defaultMessage: 'Options',
description: i18n.translate('dashboard.topNave.settingsConfigDescription', {
defaultMessage: 'Change settings for your dashboard',
Copy link
Contributor

Choose a reason for hiding this comment

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

How about "Open dashboard settings"

settings: {
...topNavStrings.settings,
id: 'settings',
testId: 'dashboardSettingsButton',
disableButton: isSaveInProgress,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should "Settings" button be disabled when the Settings flyout is open? Clicking button again cause flyout to animate open again.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I think all the nav bar items should be disabled while the flyout is open since the flyout contains local state that could be lost by clicking "Share", "Save as", or "Switch to view mode".

</EuiButtonEmpty>
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButton
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some loading state to the "Apply" button. For example, throttle network connection and click "Apply". Notice how the button does not show any loading state and is still click able while duplicate title check is occuring.

setIsTitleDuplicateConfirmed(true);
};

const onApply = async () => {
Copy link
Contributor

@nreese nreese Mar 29, 2023

Choose a reason for hiding this comment

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

Thanks for adding loading state. You will also need to ensure component is still mounted after async task finishes. Otherwise you get errors like this if the component is unmounted before async task finishes
Screen Shot 2023-03-29 at 10 21 22 AM

Here is a code example of how to set up isMounted in a functional component with hooks. Thanks @ThomThomson

const [isMounted, setIsMounted] = useState(true);
  useEffect(() => {
    return () => setIsMounted(false);
  });

onTitleDuplicate,
isTitleDuplicateConfirmed,
});
setIsApplying(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Move setIsApplying after is mounted check.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM
code review, tested in chrome

@kibana-ci
Copy link
Collaborator

💚 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 179 178 -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 450.9KB 455.4KB +4.5KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
dashboard 12 13 +1
Unknown metric groups

API count

id before after diff
dashboard 188 187 -1

ESLint disabled line counts

id before after diff
securitySolution 432 435 +3

Total ESLint disabled count

id before after diff
securitySolution 512 515 +3

History

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

Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

LGTM tested locally and thanks for updating the FTs 👍🏻

@nickpeihl nickpeihl merged commit b692e34 into elastic:main Mar 31, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 31, 2023
@KOTungseth KOTungseth mentioned this pull request Apr 25, 2023
19 tasks
KOTungseth added a commit that referenced this pull request May 16, 2023
## Summary

Adds the docs for the following 8.8 Presentation docs:

- Unified dashboard settings:
#153862
Docs preview:
https://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#add-dashboard-settings

- Add reset button: #154872
Docs preview:
https://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#reset-the-dashboard

---------

Co-authored-by: Nick Peihl <[email protected]>
Co-authored-by: Hannah Mudge <[email protected]>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 16, 2023
## Summary

Adds the docs for the following 8.8 Presentation docs:

- Unified dashboard settings:
elastic#153862
Docs preview:
https://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#add-dashboard-settings

- Add reset button: elastic#154872
Docs preview:
https://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#reset-the-dashboard

---------

Co-authored-by: Nick Peihl <[email protected]>
Co-authored-by: Hannah Mudge <[email protected]>
(cherry picked from commit 0689c63)
kibanamachine added a commit that referenced this pull request May 16, 2023
# Backport

This will backport the following commits from `main` to `8.8`:
- [[DOCS] Adds the presentation 8.8 docs
(#157765)](#157765)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Kaarina
Tungseth","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-16T19:21:01Z","message":"[DOCS]
Adds the presentation 8.8 docs (#157765)\n\n## Summary\r\n\r\nAdds the
docs for the following 8.8 Presentation docs:\r\n\r\n- Unified dashboard
settings:\r\nhttps://github.com//pull/153862\r\nDocs
preview:\r\nhttps://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#add-dashboard-settings\r\n\r\n-
Add reset button: https://github.com/elastic/kibana/pull/154872\r\nDocs
preview:\r\nhttps://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#reset-the-dashboard\r\n\r\n---------\r\n\r\nCo-authored-by:
Nick Peihl <[email protected]>\r\nCo-authored-by: Hannah Mudge
<[email protected]>","sha":"0689c638d3b3e4c8a9d00e1c2da2412a257771ce","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Docs","Team:Presentation","release_note:skip","v8.8.0","v8.9.0"],"number":157765,"url":"https://github.com/elastic/kibana/pull/157765","mergeCommit":{"message":"[DOCS]
Adds the presentation 8.8 docs (#157765)\n\n## Summary\r\n\r\nAdds the
docs for the following 8.8 Presentation docs:\r\n\r\n- Unified dashboard
settings:\r\nhttps://github.com//pull/153862\r\nDocs
preview:\r\nhttps://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#add-dashboard-settings\r\n\r\n-
Add reset button: https://github.com/elastic/kibana/pull/154872\r\nDocs
preview:\r\nhttps://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#reset-the-dashboard\r\n\r\n---------\r\n\r\nCo-authored-by:
Nick Peihl <[email protected]>\r\nCo-authored-by: Hannah Mudge
<[email protected]>","sha":"0689c638d3b3e4c8a9d00e1c2da2412a257771ce"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/157765","number":157765,"mergeCommit":{"message":"[DOCS]
Adds the presentation 8.8 docs (#157765)\n\n## Summary\r\n\r\nAdds the
docs for the following 8.8 Presentation docs:\r\n\r\n- Unified dashboard
settings:\r\nhttps://github.com//pull/153862\r\nDocs
preview:\r\nhttps://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#add-dashboard-settings\r\n\r\n-
Add reset button: https://github.com/elastic/kibana/pull/154872\r\nDocs
preview:\r\nhttps://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#reset-the-dashboard\r\n\r\n---------\r\n\r\nCo-authored-by:
Nick Peihl <[email protected]>\r\nCo-authored-by: Hannah Mudge
<[email protected]>","sha":"0689c638d3b3e4c8a9d00e1c2da2412a257771ce"}}]}]
BACKPORT-->

Co-authored-by: Kaarina Tungseth <[email protected]>
jasonrhodes pushed a commit that referenced this pull request May 17, 2023
## Summary

Adds the docs for the following 8.8 Presentation docs:

- Unified dashboard settings:
#153862
Docs preview:
https://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#add-dashboard-settings

- Add reset button: #154872
Docs preview:
https://kibana_157765.docs-preview.app.elstc.co/guide/en/kibana/master/dashboard.html#reset-the-dashboard

---------

Co-authored-by: Nick Peihl <[email protected]>
Co-authored-by: Hannah Mudge <[email protected]>
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 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:large Large Level of Effort release_note:enhancement Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard Usability] Unified Dashboard Options
6 participants