Skip to content

[Modal] Return control over useflex again, but provide unsupported debug warning #1240

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

Merged
merged 1 commit into from
Feb 16, 2020

Conversation

lubber-de
Copy link
Member

Description

This PR gives full control over the useFlex setting again.
Some combinations are not working for correct positioning, which was basically fixed by #1079 and #1096
However, the fixes ignored a possible explicit given useFlex:true and always assumes auto instead (which makes sure the positioning will work all the time)
As it turned out here there are some usecases, where people need the explicit setting of useFlex:true, i reverted the logic for the can.useFlex method but added some debug warnings instead when unsupported combinations occur, because the positioning for useFlex:true will not work when:

  • IE11 is used
  • detachable:false is used

As the default of useFlex is auto, existing code should basically work as before without any visual issues

I am totally fine if @fomantic/maintainers will disagree and would like to reject this PR and keep the current implementation because

  • there won't be visual positioning issues when using any of the above combinations
  • Users would need to enable the debug setting of their modals in order to see the message telling them why the positioning seems to be broken

If we reject this PR and keep the source as it is, we might change the docs, because useFlex:true will definately not work as intended and is currently always treated as auto

🗯 Let me know about your opinion

Testcase

Scroll down to see the button to open the moda
http://jsfiddle.net/9cf2o75b/

Screenshots

When useFlex:true is used in combination with

  • IE
    image

  • detachable: false
    image

@lubber-de lubber-de added type/feat Any feature requests or improvements lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews labels Dec 22, 2019
@lubber-de lubber-de added this to the 2.8.x milestone Dec 22, 2019
Copy link
Contributor

@exoego exoego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am OK with this PR.
I think this warning help developers who need to consider IE11, and the code is trivial enough to be maintained.

Copy link
Contributor

@prudho prudho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lubber-de lubber-de modified the milestones: 2.8.x, 2.8.4 Feb 3, 2020
@lubber-de lubber-de merged commit c512cf2 into fomantic:develop Feb 16, 2020
@lubber-de lubber-de deleted the allow_flex_detach_again branch February 16, 2020 08:34
@lubber-de lubber-de removed the state/awaiting-reviews Pull requests which are waiting for reviews label Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants