AK-3879 fix(component): fix modal dialog not shrinking to viewport height in IE11

#4712 Merged at e51d14e
Repository
Branch
mblasz/AK-3879-fix-modal-shrink-ie11
Repository
Branch
master
Author
  1. Michael Blaszczyk
Reviewers
Description

affects: @atlaskit/modal-dialog

ISSUES CLOSED: AK-3879

Releases
  • Commit status
  • Issues AK-3879
  • All tasks resolved
  • Dependencies Checking for dependencies...
  • Dependents Checking for dependents...

Comments (13)

  1. Sean Curtis

    Not a fan of this fix tbh. Feels a little hacky. I might be able to pick it up later next week and put in a better solution for it.

    • Look at a better fix
    1. Jared Crowe Account Deactivated

      I don't think it's that bad, max-height: calc(100% - 1px) is describing exactly how the modal is appearing. It's annoying that IE11 doesn't respect max-height: 100% for some reason, but I don't think it's the end of the world...

      @scurtis what's your main concern with it? Do you think there should be a better way to get this behaving correctly in IE11?

      1. Sean Curtis

        Well we've got some rules about it being 60px from the top/bottom, and this will change it to 61px. Also I'm pretty sure we could shuffle things around and make this entire bit of code more elegant. There are so many heights/max-heights in this code, and I suspect half of them could be removed.

        I think we've just got too many styles in here, and that's what is causing us to run into weird bugs like this. Simplifying things should prevent the bugs from manifesting.

        1. Jared Crowe Account Deactivated

          @scurtis do you think you could sync with @mblaszczyk-atlassian on how you'd approach this?

  2. Raja Bellebon

    Cross browser tested it - LGTM ! How do you feel about the fix @jaredcroweatlassian ?

  3. Joss Mackison Account Deactivated

    @scurtis @jaredcroweatlassian I'm happy with the calc fix -- it gets the job done, is isolated and annotated.

    Slightly tidier solution might be to have a flexMaxHeightFix mixin with a reference to the github issue as well.

    My $0.02

    Thanks for looking into this BTW @mblaszczyk-atlassian 👌

  4. Raja Bellebon

    @mblaszczyk-atlassian how critical is this issue across browsers worth to add any tests? I don't think so but better to have your advice too

    1. Michael Blaszczyk author

      I think it would only be needed in IE. Could add one in as a regression test but visual regression would probably be easier than e2e.

  5. Michael Blaszczyk author

    @scurtis @jmackison I've added a mixin and updated the positioner height to account for the -1px calc.

      1. Sean Curtis

        I'd rather not have to do this, but happy to have it like this until we can drop IE11 support 😉