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.

Sign up for GitHub

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

Jump to bottom

[Dashboard] Removing 100% as dshDashboardViewport height #54263

Merged
merged 1 commit into from
Jan 8, 2020
Merged

[Dashboard] Removing 100% as dshDashboardViewport height #54263

merged 1 commit into from
Jan 8, 2020

Conversation

majagrubic
Copy link
Contributor

@majagrubic majagrubic commented Jan 8, 2020 •
edited
Loading

Summary

Fixes: #54034

Before:
Screenshot 2020-01-08 at 14 40 30

After:
Screenshot 2020-01-08 at 14 45 02

Test fullscreen mode, expanding individual widget and expanding widget in fullscreen mode - all seems to be working fine.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@majagrubic majagrubic requested a review from a team as a code owner January 8, 2020 14:45
@majagrubic majagrubic added Feature:Dashboard Dashboard related features Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Jan 8, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@majagrubic majagrubic added v8.0.0 v7.6.0 release_note:skip Skip the PR/issue when compiling release notes labels Jan 8, 2020
@majagrubic majagrubic requested review from a team and tsullivan January 8, 2020 14:48
@ryankeairns
Copy link
Contributor

ryankeairns commented Jan 8, 2020 •
edited
Loading

Looking back, these CSS height declarations were added recently during the NP migration work by @flash1293 - #48913.

I wonder if the fix you made for the background-color (#54148) might have made the height values no longer necessary... checking out locally.

@flash1293
Copy link
Contributor

flash1293 commented Jan 8, 2020 •
edited
Loading

I didn't test, but if I remember correctly I added those because TSVB visualizations were failing if the container element would not use up the available space. @majagrubic could you test whether a dashboard with just a TSVB vis still renders correctly both in fullscreen and normal mode?

If this isn't the case maybe the stylings for TSVB have to be adjusted somehow.

@majagrubic
Copy link
Contributor Author

majagrubic commented Jan 8, 2020 •
edited
Loading

Thanks for additional context!
I tried in Chrome and FF, looks pretty good to me:
Regular:
Screenshot 2020-01-08 at 15 35 14

Fullscreen:
Screenshot 2020-01-08 at 15 35 26

flash1293
flash1293 approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 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 checking @majagrubic ! In this case the change is fine for me, maybe something else changed in the meantime

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

ryankeairns
ryankeairns approved these changes Jan 8, 2020
View reviewed changes
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM. I tried it locally and attempted to test in IE11 (to no avail), I'm good with merging it.

@majagrubic majagrubic merged commit bc640bd into elastic:master Jan 8, 2020
@majagrubic majagrubic deleted the dash-reports branch January 8, 2020 16:27
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 9, 2020
@gmmorris
Merge branch 'master' into task-manager/schema-config
3dd94d5 
* master: (23 commits)
  [Vis: Default editor] Reactify the timelion editor (elastic#52990)
  [Discover] fix histogram min interval (elastic#53979)
  [Telemetry] [Monitoring] Only retry fetching usage once monito… (elastic#54309)
  [docs][APM] Add runtime index config documentation (elastic#53907)
  [SIEM] Detection engine timeline (elastic#53783)
  Filter scripted fields preview field list to source fields (elastic#53826)
  Management - New platform api (elastic#52579)
  Reset region and Account when switching inventory (elastic#54287)
  [SIEM] [Case] Case workflow api schema (elastic#51535)
  Code coverage setup on CI (elastic#49003)
  [ML] DF Analytics Results: adds link to docs (elastic#54189)
  Update schemas boolean, byteSize, and duration to coerce strings (elastic#54177)
  [Metrics UI] Pass relevant shouldAllowEdit capabilities into SettingsPage (elastic#49781)
  [Canvas] Fixes bugs with autoplay and refresh (elastic#53149)
  [ML] DF Analytics Classification: ensure confusion matrix can be fetched (elastic#53629)
  Fix Vega react eslint errors (elastic#54259)
  Remove non existing codeowners (elastic#54274)
  use correct type (elastic#54244)
  [Dashboard] Removing 100% as dshDashboardViewport height (elastic#54263)
  add `examples/` to no-restricted-path config (elastic#54252)
  ...
@majagrubic majagrubic mentioned this pull request Jan 9, 2020
Merged
majagrubic pushed a commit to majagrubic/kibana that referenced this pull request Jan 9, 2020
[Dashboard] Removing 100% as dshDashboardViewport height (elastic#54263)
5ecedef
majagrubic pushed a commit that referenced this pull request Jan 9, 2020
[Dashboard] Removing 100% as dshDashboardViewport height (#54263) (#5…
e36f811 
…4336)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryankeairns ryankeairns ryankeairns approved these changes

@flash1293 flash1293 flash1293 approved these changes

@tsullivan tsullivan Awaiting requested review from tsullivan

Assignees
No one assigned
Labels
Feature:Dashboard Dashboard related features release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.6.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Reporting/Dashboard] Dashboard Reports have gap at the bottom
5 participants
@majagrubic @elasticmachine @ryankeairns @flash1293 @kibanamachine