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

Height:100% for welcome pages on Safari #3340

Merged
merged 2 commits into from Mar 8, 2017

Conversation

@lukebarnard1
Copy link
Contributor

lukebarnard1 commented Mar 1, 2017

Fix for #3298

This seems to not have any side-effects.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Mar 2, 2017

This sounds like a worryingly invasive change, in terms of changing the
whole height management CSS of the app, which I had absolute hell
sorting out in the initial incarnations of vector. Particularly I think
it might break when the status bar is visible at the top. I'll have a play.

@@ -100,6 +100,11 @@ limitations under the License.
* flex itself.

This comment has been minimized.

Copy link
@ara4n

ara4n Mar 2, 2017

Member

Right, I even commented this: "Height has to be auto here for RoomView to correctly fit when the Toolbar is shown". This is why it's not set to 100%. Unless the phase of the moon has changed since I wrote this, I'd expect it still to be true.

Luke: have you tested this with the MatrixToolbar visible across the top of the page (e.g. showing 'you're a guest' or 'new version' text)?

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Mar 2, 2017

Author Contributor

I have now and as far as I can tell, it's fine. (Albeit I'm testing with Chrome)

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Mar 2, 2017

Author Contributor

TODO: Test on Firefox and Safari

This comment has been minimized.

Copy link
@lukebarnard1

lukebarnard1 Mar 3, 2017

Author Contributor

Tests done for Chrome, Safari, Firefox on macOS - seems to work without upsetting anything

@ara4n ara4n assigned lukebarnard1 and unassigned ara4n Mar 2, 2017
@lukebarnard1 lukebarnard1 assigned ara4n and unassigned lukebarnard1 Mar 3, 2017
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Mar 3, 2017

lgtm then. please fix the comment that says "height must be auto!!!!" before merging.

Having tested Riot with the middlePanel having a height of 100%, it seems to be OK.
@lukebarnard1 lukebarnard1 merged commit 6a11182 into develop Mar 8, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.