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

Calculate viewport dimensions from the scope-app div #2473

Merged
merged 2 commits into from
Apr 27, 2017

Conversation

fbarl
Copy link
Contributor

@fbarl fbarl commented Apr 25, 2017

Fixes #2472 by calculating the viewport dimensions from the scope-app div bounding rectangle instead of window.width & window.height (the changes were pulled out from PR #2420).

@fbarl fbarl self-assigned this Apr 25, 2017
@fbarl fbarl requested a review from foot April 25, 2017 15:04
// Use the `scope-app` div to determine the viewport rectangle, because `window` gives
// wrong values for scope container inside Weave Cloud (on dev & prod), because of the
// top navigation toolbar that is counted in, even though it's not part of Scope.
const viewport = document.getElementsByClassName('scope-app')[0].getBoundingClientRect();

This comment was marked as abuse.

This comment was marked as abuse.

// wrong values for scope container inside Weave Cloud (on dev & prod), because of the
// top navigation toolbar that is counted in, even though it's not part of Scope.
const viewport = document.getElementsByClassName('scope-app')[0].getBoundingClientRect();
// Not sure why we need to subtract `left` and `top`, but that gives us correct values.

This comment was marked as abuse.

This comment was marked as abuse.

@fbarl fbarl force-pushed the fix-viewport-dimensions-calculation branch from b97269d to 763b924 Compare April 26, 2017 13:21
@fbarl
Copy link
Contributor Author

fbarl commented Apr 26, 2017

@foot PTAL

@foot
Copy link
Contributor

foot commented Apr 27, 2017

I don't think this fixes #2472 anymore. Tested with service-ui@master:

SERVICE_HOST=frontend.dev.weave.works PROXY_SCOPE_COMPONENT=1 SCOPE_PATH=/Users/simon/weave/scope npm start

I (think) I can still fix this by fiddling w/ the height though, what problems did you have with it?
This is the change I made (done inline as scss doesn't work w/ PROXY_SCOPE_COMPONENT for now).

diff --git a/client/app/scripts/components/app.js b/client/app/scripts/components/app.js
index 6977c0f1..19eddfdc 100644
--- a/client/app/scripts/components/app.js
+++ b/client/app/scripts/components/app.js
@@ -151,7 +151,7 @@ class App extends React.Component {
     const isIframe = window !== window.top;

     return (
-      <div className="scope-app" ref={this.saveAppRef}>
+      <div className="scope-app" style={{height: 'auto'}} ref={this.saveAppRef}>
         {showingDebugToolbar() && <DebugToolbar />}

         {showingHelp && <HelpPanel />}

@fbarl
Copy link
Contributor Author

fbarl commented Apr 27, 2017

Ups, I forgot to update the description, sorry for that. Basically this PR is only the first step towards fixing it, the second being adding a line or two to service-ui stylesheets.

What I suggested in #2473 (comment) was to add height: calc(100% - #{$top-toolbar-height}) but your height: auto would also work and it's more elegant, so I'd go with that.

The fact remains that it needs to be added in a separate PR, so I think I'll just do it when updating Scope UI (later today).

@fbarl fbarl force-pushed the fix-viewport-dimensions-calculation branch from 763b924 to d68af37 Compare April 27, 2017 12:29
@fbarl fbarl force-pushed the fix-viewport-dimensions-calculation branch from d68af37 to e77f396 Compare April 27, 2017 13:24
@fbarl
Copy link
Contributor Author

fbarl commented Apr 27, 2017

@foot Mea culpa again, your height: auto solution in Scope stylesheets works fine, so nothing needs to be added to service-ui stylesheets, this PR alone should resolve #2472!

@foot
Copy link
Contributor

foot commented Apr 27, 2017

lgtm!

@fbarl fbarl merged commit 206a88d into master Apr 27, 2017
@fbarl fbarl deleted the fix-viewport-dimensions-calculation branch June 16, 2017 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants