Skip to content

[Dashboard] Removing 100% as dshDashboardViewport height#54263

Merged
majagrubic merged 1 commit intoelastic:masterfrom
majagrubic:dash-reports
Jan 8, 2020
Merged

[Dashboard] Removing 100% as dshDashboardViewport height#54263
majagrubic merged 1 commit intoelastic:masterfrom
majagrubic:dash-reports

Conversation

@majagrubic
Copy link
Copy Markdown
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 Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Jan 8, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Jan 8, 2020

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

kibanamachine commented Jan 8, 2020

💚 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
Copy Markdown
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 Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// 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