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 Reset button disable #159430

Merged
merged 4 commits into from Jun 12, 2023
Merged

Conversation

AbrarAlHasan
Copy link
Contributor

@AbrarAlHasan AbrarAlHasan commented Jun 10, 2023

Summary

Closes #159422

Summarize your PR. If it involves visual changes include a screenshot or gif.
Added Reset Disable for Title and Description.

Cases

  1. If the title or Description is same and not changed Reset button is diabled.
  2. If the Reset is clicked then the title or description resets to the Previous value before Saved
  3. [
Screen.Recording.2023-06-10.at.11.21.35.AM.1.mp4

](url)

For maintainers

@AbrarAlHasan AbrarAlHasan requested a review from a team as a code owner June 10, 2023 06:17
@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?

@botelastic botelastic bot added the Feature:Embedding Embedding content via iFrame label Jun 10, 2023
@cla-checker-service
Copy link

cla-checker-service bot commented Jun 10, 2023

💚 CLA has been signed

@ThomThomson
Copy link
Contributor

@AbrarAlHasan would you mind signing the CLA? If you do, I can kick off a build.

@AbrarAlHasan
Copy link
Contributor Author

AbrarAlHasan commented Jun 12, 2023

@AbrarAlHasan would you mind signing the CLA? If you do, I can kick off a build.

Sure. I have Signed it

@ThomThomson ThomThomson added release_note:fix Feature:Dashboard Dashboard related features Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas 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. labels Jun 12, 2023
@elasticmachine
Copy link
Contributor

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

@ThomThomson
Copy link
Contributor

@buildkite test this

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @AbrarAlHasan!

The reset button is actually meant to only reset to the default title. This allows the dashboard author to remove the custom title and fall back directly to the saved object title. To see this behaviour you'll need to use a panel linked to a library item (use the add from Library button on the Dashboard).

The way you've written this is equivalent to resetting to the last saved custom title, which can be done just by clicking cancel instead of save in the bottom of the flyout. I've made some suggested changes which make the buttons disabled when the current title / description is equal to the default title / description.

Comment on lines 128 to 130
onClick={() => {
setPanelTitle(embeddable.getInput().title ?? embeddable.getOutput().defaultTitle);
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be undone: the reset button should always set it back to the default title.

Suggested change
onClick={() => {
setPanelTitle(embeddable.getInput().title ?? embeddable.getOutput().defaultTitle);
}}
onClick={() => setPanelTitle(embeddable.getOutput().defaultTitle)}

Comment on lines 131 to 135
disabled={
hideTitle || !editMode || embeddable.getInput().title != undefined
? embeddable.getInput().title === panelTitle
: embeddable.getOutput().defaultTitle === panelTitle || panelTitle === ''
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only be disabled if the panel title is equal to the default title:

Suggested change
disabled={
hideTitle || !editMode || embeddable.getInput().title != undefined
? embeddable.getInput().title === panelTitle
: embeddable.getOutput().defaultTitle === panelTitle || panelTitle === ''
}
disabled={
hideTitle || !editMode || embeddable.getOutput().defaultTitle === panelTitle
}

Comment on lines 179 to 181
setPanelDescription(
embeddable.getInput().description ?? embeddable.getOutput().defaultDescription
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change should be undone: the reset button should always set it back to the default description.

Suggested change
setPanelDescription(
embeddable.getInput().description ?? embeddable.getOutput().defaultDescription
);
setPanelDescription(embeddable.getOutput().defaultDescription);

Comment on lines 183 to 188
disabled={
hideTitle || !editMode || embeddable.getInput().description != undefined
? embeddable.getInput().description === panelDescription
: embeddable.getOutput().defaultDescription === panelDescription ||
panelDescription === ''
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should only be disabled if the panel description is equal to the default description:

Suggested change
disabled={
hideTitle || !editMode || embeddable.getInput().description != undefined
? embeddable.getInput().description === panelDescription
: embeddable.getOutput().defaultDescription === panelDescription ||
panelDescription === ''
}
disabled={
hideTitle ||
!editMode ||
embeddable.getOutput().defaultDescription === panelDescription
}

@AbrarAlHasan
Copy link
Contributor Author

I apologize for the misunderstanding. Thank you for clarifying the intended behavior of the reset button. I will make the necessary changes to ensure that it only resets to the default title and functions as intended with panels linked to library items.I will incorporate these changes into the code to align with the intended behavior.

@ThomThomson
Copy link
Contributor

buildkite test this

@ThomThomson
Copy link
Contributor

buildkite test this

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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
embeddable 75.9KB 75.9KB +70.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 410 414 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 491 495 +4
total +6

History

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

@ThomThomson
Copy link
Contributor

@elasticmachine run elasticsearch-ci/docs

@ThomThomson ThomThomson merged commit 1349d0a into elastic:main Jun 12, 2023
16 checks passed
@kibanamachine kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels Jun 12, 2023
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 13, 2023
* main: (199 commits)
  [Lens] Add custom formatter within the Lens editor (elastic#158468)
  [Uptime] Hide app if no data is available (elastic#159118)
  [CodeEditor] Add grok highlighting (elastic#159334)
  Expose decoded cloudId components from the cloud plugin's contract (elastic#159442)
  [Profiling] Use collector and symbolizer integrations in the add data page (elastic#159481)
  [Infrastructure UI] Hosts View: Unified Search bar with auto-refresh enabled (elastic#157011)
  [APM] Add feature flag for not available apm schema (elastic#158911)
  [Lens] Remove deprecated componentWillReceiveProps usage (elastic#159502)
  [api-docs] 2023-06-13 Daily api_docs build (elastic#159536)
  [data views] Use versioned router for REST routes (elastic#158608)
  [maps] fix geo line source not loaded unless maps application is opened (elastic#159432)
  [Enterprise Search][Search application]Fix Create Api key url (elastic#159519)
  [Security Solution] Increase timeout for indexing hosts (elastic#159518)
  dashboard Reset button disable (elastic#159430)
  [Security Solution] Unskip Endpoint API tests after package fix (elastic#159484)
  [Synthetics] adjust alert timing (elastic#159511)
  [ResponseOps][rule registry] Remove usages of `refresh: true` (elastic#159252)
  Revert "Remove unused package (elastic#158597)"
  [Serverless] Adding config to disable authentication on task manager background worker utilization API (elastic#159505)
  Remove unused package (elastic#158597)
  ...
saarikabhasi pushed a commit to saarikabhasi/kibana that referenced this pull request Jun 13, 2023
Disables reset button on title and description when they are the same as default.
saarikabhasi pushed a commit to saarikabhasi/kibana that referenced this pull request Jun 13, 2023
Disables reset button on title and description when they are the same as default.
saarikabhasi pushed a commit to saarikabhasi/kibana that referenced this pull request Jun 14, 2023
Disables reset button on title and description when they are the same as default.
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 Feature:Embedding Embedding content via iFrame 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.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User shouldn't be able to click reset on panel settings for dashboard panels without making any changes
5 participants