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

[Finishes #98663750] disable responsive styling for IE9 #4455

Merged
merged 2 commits into from Oct 9, 2015
Merged

Conversation

@Bjvanminnen
Copy link
Contributor

Bjvanminnen commented Oct 8, 2015

IE9 was throwing when trying to dynamically modify our styles to support responsiveness.

What I've done here is add a util method to discover that case. If media rules aren't supported, we exit adjustAppSizeStyles.

I then extracted some of what was happening in onMouseMoveVizResizeBar into resizeVisualization, allowing us to explicitly call resize in the case where we couldn't set up our responsiveness. Finally, when doing our resize we'll set height in addition to max-height if we're in this case.

Things I've tested:
In Chrome - resize window, make sure play space resizes properly. Drag grippy, playspace resizes. Resize window again - responsiveness still works.

In IE9 mode on IE11 - page loads. Resizing window does not change play space area. Moving grippy around does.

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor Author

Bjvanminnen commented Oct 8, 2015

Initially I wanted to commit this along with enabling a bunch of UI tests for IE9. Turns out, our AppLab UI tests don't even work on IE10, and despite many hours of work, I've been unable to get them to thus far. Eventually I decided to get this work in, and created a separate bug to get UI tests enabled. I also created a bug to track the fact that web workers (background linting) doesn't work.

There may still be other IE9 bugs to, but this at least gets us initially unblocked.

@@ -667,6 +671,12 @@ Applab.init = function(config) {
drawDiv();

studioApp.alertIfAbusiveProject('#codeWorkspace');

// IE9 doesnt support the way we handle responsiveness. Instead, expclitily

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Oct 9, 2015

Contributor

explicitly

// We don't get the benefits of our responsive styling, so set height
// explicitly
if (!utils.browserSupportsCssMedia()) {
visualization.style.height = newVizHeightString;

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Oct 9, 2015

Contributor

why set height and not width?

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Oct 9, 2015

Author Contributor

The short answer is that it's because height is what was broken, whereas i wasn't seeing any problems with width. I'll do a little more due diligence and see if it makes sense to be setting width too.

This comment has been minimized.

Copy link
@davidsbailey

davidsbailey Oct 9, 2015

Contributor

OK. My guess is that you only needed to set the height because the default size is 400 x 400 and the applab-specified size is 320 x 450 (w x h). so the width was already "big enough". So the concern would be that if applab ever specified a level with width > 400. Seems unlikely, but might be worth just setting width now as it could save us having to debug in IE9 to discover that this is the problem if we ever do make a wider applab level.

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Oct 9, 2015

Author Contributor

Yeah, I think you're right - both in your analysis and suggested solution :)

@davidsbailey

This comment has been minimized.

Copy link
Contributor

davidsbailey commented Oct 9, 2015

LGTM

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor Author

Bjvanminnen commented Oct 9, 2015

note: circle ci failure is a timeout in a test that we've since bumped the timeout for

Bjvanminnen added a commit that referenced this pull request Oct 9, 2015
[Finishes #98663750] disable responsive styling for IE9
@Bjvanminnen Bjvanminnen merged commit 1400b44 into staging Oct 9, 2015
3 of 4 checks passed
3 of 4 checks passed
ci/circleci Your tests failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
hound No violations found. Woof!
@Bjvanminnen Bjvanminnen deleted the fixIE9 branch Oct 9, 2015
deploy-code-org added a commit that referenced this pull request Oct 9, 2015
commit 1400b44
Merge: 3d6fce2 cc51c9b
Author: Bjvanminnen <Bjvanminnen@gmail.com>
Date:   Fri Oct 9 11:50:31 2015 -0700

    Merge pull request #4455 from code-dot-org/fixIE9

    [Finishes #98663750] disable responsive styling for IE9

commit 3d6fce2
Author: Continuous Integration <dev@code.org>
Date:   Fri Oct 9 18:49:04 2015 +0000

    Automatically built.

    commit 9de3537
    Author: Josh Lory <josh.lory@code.org>
    Date:   Fri Oct 9 11:02:54 2015 -0700

        [Finishes #96026028] Remove extra scrollbar in Asset Manager

commit 9de3537
Author: Josh Lory <josh.lory@code.org>
Date:   Fri Oct 9 11:02:54 2015 -0700

    [Finishes #96026028] Remove extra scrollbar in Asset Manager
deploy-code-org added a commit that referenced this pull request Oct 9, 2015
commit 2ecd2b8
Merge: 6d3bbec e81386c
Author: Brendan Reville <breville@users.noreply.github.com>
Date:   Fri Oct 9 13:49:37 2015 -0700

    Merge pull request #4443 from code-dot-org/hoc2015smallupdate

    hoc2015: small updates

commit 6d3bbec
Merge: 77970f7 1518c31
Author: Mehal Shah <mehal@code.org>
Date:   Fri Oct 9 13:35:06 2015 -0700

    Merge pull request #4472 from code-dot-org/specify_path_in_cookies_2

    Specify path when setting cookies

commit 1518c31
Author: Mehal Shah <mehal@code.org>
Date:   Fri Oct 9 12:44:45 2015 -0700

    Specify path when setting cookies

commit e81386c
Author: Brendan Reville <brendan@code.org>
Date:   Fri Oct 9 13:32:52 2015 -0700

    hoc2015: draw walls correct using either level.wallMap or level.map.

    Now draws walls correctly for any of the following situations:
      - level.map specifies random tiles, drawn once at level load. (e.g. level 1)
      - level.wallMap specifies a skin layout, drawn upfront and redrawn when background changed, if necessary. (e.g. level 14)
      - a hybrid in which level.map specifies sprite/item locations, while level.wallMap also specifies a skin-based wall layout. (e.g. level 7)
      - when level.wallMap is set and a student calls setMap.

    It gets rid of the defaut skin wall map in hoc2015 skin ("blank") because this made the first situation difficult to deal with.  Now, when there is no skin-based map specified, setMap doesn't do anything, so it doesn't go about erasing the random tiles that are drawn once-only.  (They are drawn once-only to avoid having the randomly-chosen tiles change.  An alternative would have been to use a non-truly-random layout.)

    There is no longer

commit 77970f7
Author: Josh Lory <josh.lory@code.org>
Date:   Fri Oct 9 12:56:35 2015 -0700

    Turn off rubocop metric rules

commit 4675ca3
Merge: 3092603 4b96356
Author: Josh Lory <josh.lory@code.org>
Date:   Fri Oct 9 12:52:06 2015 -0700

    Merge pull request #4445 from code-dot-org/update-rubocop

    Update rubocop to 0.34.2 and regenerate TODO list

commit 3092603
Author: Continuous Integration <dev@code.org>
Date:   Fri Oct 9 18:59:57 2015 +0000

    Automatically built.

    commit 1400b44
    Merge: 3d6fce2 cc51c9b
    Author: Bjvanminnen <Bjvanminnen@gmail.com>
    Date:   Fri Oct 9 11:50:31 2015 -0700

        Merge pull request #4455 from code-dot-org/fixIE9

        [Finishes #98663750] disable responsive styling for IE9

    commit 3d6fce2
    Author: Continuous Integration <dev@code.org>
    Date:   Fri Oct 9 18:49:04 2015 +0000

        Automatically built.

        commit 9de3537
        Author: Josh Lory <josh.lory@code.org>
        Date:   Fri Oct 9 11:02:54 2015 -0700

            [Finishes #96026028] Remove extra scrollbar in Asset Manager

    commit 9de3537
    Author: Josh Lory <josh.lory@code.org>
    Date:   Fri Oct 9 11:02:54 2015 -0700

        [Finishes #96026028] Remove extra scrollbar in Asset Manager

commit 1400b44
Merge: 3d6fce2 cc51c9b
Author: Bjvanminnen <Bjvanminnen@gmail.com>
Date:   Fri Oct 9 11:50:31 2015 -0700

    Merge pull request #4455 from code-dot-org/fixIE9

    [Finishes #98663750] disable responsive styling for IE9

commit 3d6fce2
Author: Continuous Integration <dev@code.org>
Date:   Fri Oct 9 18:49:04 2015 +0000

    Automatically built.

    commit 9de3537
    Author: Josh Lory <josh.lory@code.org>
    Date:   Fri Oct 9 11:02:54 2015 -0700

        [Finishes #96026028] Remove extra scrollbar in Asset Manager

commit 9de3537
Author: Josh Lory <josh.lory@code.org>
Date:   Fri Oct 9 11:02:54 2015 -0700

    [Finishes #96026028] Remove extra scrollbar in Asset Manager

commit a8bdaad
Merge: c78ce1d d51d422
Author: Mehal Shah <mehal@code.org>
Date:   Fri Oct 9 11:38:28 2015 -0700

    Merge pull request #4460 from code-dot-org/ui_tests_hoc_ie_workaround

    Workaround for UI test improvements in IE - do the second test on the…

commit cc51c9b
Author: Brent Van Minnen <bjvanminnen@gmail.com>
Date:   Fri Oct 9 11:30:56 2015 -0700

    code review feedback
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.