-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix the calculation notebook view #159
Conversation
The view was previously limited to a height of 60px. The notebook height is now determined by the available space. Signed-off-by: Brianna Major <[email protected]>
Addresses #150 |
src/components/notebook.js
Outdated
const appStyles = theme => ({ | ||
iframe: { | ||
width: '100%', | ||
height: '-webkit-fill-available' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work on non webkit based browser? And do we care ? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will not, good point. I've updated the styling to calculate from the size of the notebook instead and this seems to work well in both Chrome and Firefox.
To ensure cross-browser support, calculate the height of the IFrame based on the height of the notebook rather than using webkit styling. Signed-off-by: Brianna Major <[email protected]>
@bnmajor This works, but there a bit of delay, while the size is calculated. So it looks like this for a second or so: Is there a way we could do it with CSS across browsers or hidden the component util the calculation is done. |
Use the notebook aspect ratio to determine padding for the iFrame container. This keeps the proportions of the JupyterLab Notebook, avoids the delay caused by calculating the height, and seems to work smoothly across browsers. Signed-off-by: Brianna Major <[email protected]>
src/components/notebook.js
Outdated
container: { | ||
overflow: 'hidden', | ||
position: 'relative', | ||
paddingTop: '72%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let give ourselves a little more height and increase this to 100%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
Signed-off-by: Brianna Major <[email protected]>
The view was previously limited to a height of 60px. The notebook height is now
determined by the available space.
Signed-off-by: Brianna Major [email protected]