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] Add scroll margin to panels #193430

Merged
merged 15 commits into from
Oct 7, 2024

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Sep 19, 2024

Summary

Closes [Bug] Upper part of the chart is hidden when user edits a chart inline · Issue #192437 · elastic/kibana · GitHub

This PR disables the overflow-y hidden css and the panel is set to the top. Users are also able to scroll to the top of the visualization in inline editing mode.

Now:

September.-.19.mp4

@rshen91 rshen91 self-assigned this Sep 19, 2024
@rshen91 rshen91 added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:fix backport:all-open Backport to all branches that could still receive a release labels Sep 19, 2024
@rshen91 rshen91 marked this pull request as ready for review September 20, 2024 13:42
@rshen91 rshen91 requested review from a team as code owners September 20, 2024 13:42
@elasticmachine
Copy link
Contributor

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

@rshen91 rshen91 added the fixed label Sep 20, 2024
@Heenawter Heenawter self-requested a review September 20, 2024 15:32
Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

I don't think this is a good solution:

Screen.Recording.2024-09-20.at.9.52.03.AM.mov

I think we should instead just add an offset to the scrolIntoView somehow (I assume that is possible, but I haven't confirmed)

@rshen91 rshen91 requested a review from a team as a code owner September 20, 2024 16:10
@rshen91
Copy link
Contributor Author

rshen91 commented Sep 20, 2024

I don't think this is a good solution:

Screen.Recording.2024-09-20.at.9.52.03.AM.mov
I think we should instead just add an offset to the scrolIntoView somehow (I assume that is possible, but I haven't confirmed)

Ah ok - yeah I don't know if this is working super well for long panels that extend past the viewport dimensions, but by overwriting the css at least users are able to scroll up and see the top of the panel. I think your video showed a good reason to not do scrollToTop. How do you feel about scrollIntoView({ block: 'nearest'}) ?

Comment on lines 57 to 59
.coreSystemRootDomElement {
overflow-y: auto !important;
}
Copy link
Contributor

@Heenawter Heenawter Sep 20, 2024

Choose a reason for hiding this comment

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

I believe allowing y-overflow leads to some strange behaviour when trying to scroll when both the editor + dashboard have scrollbars:

Screen.Recording.2024-09-20.at.11.26.01.AM.mov

I will default to @cqliu1's or @andreadelrio's opinion here, but I think the best solution might be to just change the scroll into view logical position method? 🤔 And nearest seems to work well IMO

Copy link
Contributor

@Heenawter Heenawter Sep 23, 2024

Choose a reason for hiding this comment

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

fyi I stumbled across the scroll-margin CSS property - and it seems like it will be a good helper here even without enabling overflow-y 👍

@rshen91 rshen91 requested a review from Heenawter October 1, 2024 20:06
@rshen91 rshen91 requested review from a team as code owners October 3, 2024 19:23
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 8, 2024
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

3 similar comments
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@maryam-saeidi
Copy link
Member

@rshen91 It seems some of the backports failed for this PR. If this PR doesn't need to be backported, would you mind adjusting the labels to avoid notification?

@rshen91 rshen91 added backport:current-major Backport to all previous minor branches and removed backport missing Added to PRs automatically when the are determined to be missing a backport. backport:all-open Backport to all branches that could still receive a release labels Oct 14, 2024
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it 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 15, 2024
@rshen91
Copy link
Contributor Author

rshen91 commented Oct 15, 2024

💚 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

rshen91 added a commit to rshen91/kibana that referenced this pull request Oct 15, 2024
## Summary

Closes [[Bug] Upper part of the chart is hidden when user edits a chart
inline · Issue elastic#192437 · elastic/kibana ·
GitHub](elastic#192437)

This PR disables the overflow-y hidden css and the panel is set to the
top. Users are also able to scroll to the top of the visualization in
inline editing mode.

### Now:

https://github.com/user-attachments/assets/a68a3bdc-5452-40f3-84a6-4950e2fc7893

---------

Co-authored-by: Hannah Mudge <[email protected]>
(cherry picked from commit b4d52e4)

# Conflicts:
#	src/plugins/dashboard/public/dashboard_api/track_panel.ts
kibanamachine added a commit that referenced this pull request Oct 15, 2024
…195323)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Dashboard] [Usability] Add scroll margin to panels
(#193430)](#193430)

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

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

<!--BACKPORT [{"author":{"name":"Rachel
Shen","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-07T20:43:40Z","message":"[Dashboard]
[Usability] Add scroll margin to panels (#193430)\n\n##
Summary\r\n\r\nCloses [[Bug] Upper part of the chart is hidden when user
edits a chart\r\ninline · Issue #192437 · elastic/kibana
·\r\nGitHub](https://github.com/elastic/kibana/issues/192437)\r\n\r\nThis
PR disables the overflow-y hidden css and the panel is set to
the\r\ntop. Users are also able to scroll to the top of the
visualization in\r\ninline editing mode.\r\n\r\n### Now:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a68a3bdc-5452-40f3-84a6-4950e2fc7893\r\n\r\n---------\r\n\r\nCo-authored-by:
Hannah Mudge
<[email protected]>","sha":"b4d52e440ef6f7c68675893cde4994a2b3d45247","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","v9.0.0","fixed","backport:all-open"],"title":"[Dashboard]
[Usability] Add scroll margin to
panels","number":193430,"url":"https://github.com/elastic/kibana/pull/193430","mergeCommit":{"message":"[Dashboard]
[Usability] Add scroll margin to panels (#193430)\n\n##
Summary\r\n\r\nCloses [[Bug] Upper part of the chart is hidden when user
edits a chart\r\ninline · Issue #192437 · elastic/kibana
·\r\nGitHub](https://github.com/elastic/kibana/issues/192437)\r\n\r\nThis
PR disables the overflow-y hidden css and the panel is set to
the\r\ntop. Users are also able to scroll to the top of the
visualization in\r\ninline editing mode.\r\n\r\n### Now:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a68a3bdc-5452-40f3-84a6-4950e2fc7893\r\n\r\n---------\r\n\r\nCo-authored-by:
Hannah Mudge
<[email protected]>","sha":"b4d52e440ef6f7c68675893cde4994a2b3d45247"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193430","number":193430,"mergeCommit":{"message":"[Dashboard]
[Usability] Add scroll margin to panels (#193430)\n\n##
Summary\r\n\r\nCloses [[Bug] Upper part of the chart is hidden when user
edits a chart\r\ninline · Issue #192437 · elastic/kibana
·\r\nGitHub](https://github.com/elastic/kibana/issues/192437)\r\n\r\nThis
PR disables the overflow-y hidden css and the panel is set to
the\r\ntop. Users are also able to scroll to the top of the
visualization in\r\ninline editing mode.\r\n\r\n### Now:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a68a3bdc-5452-40f3-84a6-4950e2fc7893\r\n\r\n---------\r\n\r\nCo-authored-by:
Hannah Mudge
<[email protected]>","sha":"b4d52e440ef6f7c68675893cde4994a2b3d45247"}}]}]
BACKPORT-->

Co-authored-by: Rachel Shen <[email protected]>
rshen91 added a commit that referenced this pull request Oct 15, 2024
…196382)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[Dashboard] [Usability] Add scroll margin to panels
(#193430)](#193430)

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

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

<!--BACKPORT [{"author":{"name":"Rachel
Shen","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-07T20:43:40Z","message":"[Dashboard]
[Usability] Add scroll margin to panels (#193430)\n\n##
Summary\r\n\r\nCloses [[Bug] Upper part of the chart is hidden when user
edits a chart\r\ninline · Issue #192437 · elastic/kibana
·\r\nGitHub](https://github.com/elastic/kibana/issues/192437)\r\n\r\nThis
PR disables the overflow-y hidden css and the panel is set to
the\r\ntop. Users are also able to scroll to the top of the
visualization in\r\ninline editing mode.\r\n\r\n### Now:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a68a3bdc-5452-40f3-84a6-4950e2fc7893\r\n\r\n---------\r\n\r\nCo-authored-by:
Hannah Mudge
<[email protected]>","sha":"b4d52e440ef6f7c68675893cde4994a2b3d45247","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","Team:Presentation","backport
missing","v9.0.0","fixed","backport:current-major"],"number":193430,"url":"https://github.com/elastic/kibana/pull/193430","mergeCommit":{"message":"[Dashboard]
[Usability] Add scroll margin to panels (#193430)\n\n##
Summary\r\n\r\nCloses [[Bug] Upper part of the chart is hidden when user
edits a chart\r\ninline · Issue #192437 · elastic/kibana
·\r\nGitHub](https://github.com/elastic/kibana/issues/192437)\r\n\r\nThis
PR disables the overflow-y hidden css and the panel is set to
the\r\ntop. Users are also able to scroll to the top of the
visualization in\r\ninline editing mode.\r\n\r\n### Now:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a68a3bdc-5452-40f3-84a6-4950e2fc7893\r\n\r\n---------\r\n\r\nCo-authored-by:
Hannah Mudge
<[email protected]>","sha":"b4d52e440ef6f7c68675893cde4994a2b3d45247"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","labelRegex":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193430","number":193430,"mergeCommit":{"message":"[Dashboard]
[Usability] Add scroll margin to panels (#193430)\n\n##
Summary\r\n\r\nCloses [[Bug] Upper part of the chart is hidden when user
edits a chart\r\ninline · Issue #192437 · elastic/kibana
·\r\nGitHub](https://github.com/elastic/kibana/issues/192437)\r\n\r\nThis
PR disables the overflow-y hidden css and the panel is set to
the\r\ntop. Users are also able to scroll to the top of the
visualization in\r\ninline editing mode.\r\n\r\n### Now:
\r\n\r\n\r\nhttps://github.com/user-attachments/assets/a68a3bdc-5452-40f3-84a6-4950e2fc7893\r\n\r\n---------\r\n\r\nCo-authored-by:
Hannah Mudge
<[email protected]>","sha":"b4d52e440ef6f7c68675893cde4994a2b3d45247"}},{"url":"https://github.com/elastic/kibana/pull/195323","number":195323,"branch":"8.x","state":"OPEN"}]}]
BACKPORT-->
@kibanamachine kibanamachine added v8.15.3 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Oct 15, 2024
@Heenawter Heenawter added backport:version Backport to applied version labels and removed backport:current-major Backport to all previous minor branches labels Oct 15, 2024
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.15, 8.x

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

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.15 Backport failed because of merge conflicts

You might need to backport the following PRs to 8.15:
- [Reporting] Improvements to reporting diagnostics (#191790)
8.x Cherrypick failed because the selected commit (b4d52e4) is empty. It looks like the commit was already backported in #195323

Manual backport

To create the backport manually run:

node scripts/backport --pr 193430

Questions ?

Please refer to the Backport tool documentation

@Heenawter Heenawter removed the backport:version Backport to applied version labels label Oct 15, 2024
@mistic
Copy link
Member

mistic commented Oct 17, 2024

This PR didn't make it into the latest BC of v8.15.3. Updating the labels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixed release_note:fix Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.15.4 v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] Upper part of the chart is hidden when user edits a chart inline
8 participants